Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Use movsdsse2 for HW intrinsic SSE2_ConvertToDouble#17110

Merged
AndyAyersMS merged 1 commit into
dotnet:masterfrom
AndyAyersMS:FixMovsd
Mar 26, 2018
Merged

Use movsdsse2 for HW intrinsic SSE2_ConvertToDouble#17110
AndyAyersMS merged 1 commit into
dotnet:masterfrom
AndyAyersMS:FixMovsd

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

Instead of movsd, which is for GPRs. Fixes one of the stress failures
seen in #17027.

Instead of movsd, which is for GPRs. Fixes one of the stress failures
seen in #17027.
@AndyAyersMS
Copy link
Copy Markdown
Member Author

@4creators @fiigii @tannergooding PTAL
cc @dotnet/jit-contrib

Feel free to add whatever other legs are appropriate, I'm not familiar with most of them.

@dotnet-bot test Windows_NT x64 Checked jitstressregs4
@dotnet-bot test Windows_NT x86 Checked jitstressregs4

Copy link
Copy Markdown

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks!

@4creators
Copy link
Copy Markdown

test Windows_NT x64 Checked gcstress0x3
test Windows_NT x64 Checked gcstress0xc
test Windows_NT x64 Checked gcstress0xc_jitstress1
test Windows_NT x64 Checked gcstress0xc_jitstress2
test Windows_NT x64 Checked gcstress0xc_minopts_heapverify1
test Windows_NT x64 Checked gcstress0xc_zapdisable
test Windows_NT x64 Checked gcstress0xc_zapdisable_heapverify1
test Windows_NT x64 Checked gcstress0xc_zapdisable_jitstress2

test Windows_NT x86 Checked gcstress0x3
test Windows_NT x86 Checked gcstress0xc
test Windows_NT x86 Checked gcstress0xc_jitstress1
test Windows_NT x86 Checked gcstress0xc_jitstress2
test Windows_NT x86 Checked gcstress0xc_minopts_heapverify1
test Windows_NT x86 Checked gcstress0xc_zapdisable
test Windows_NT x86 Checked gcstress0xc_zapdisable_heapverify1
test Windows_NT x86 Checked gcstress0xc_zapdisable_jitstress2

test Ubuntu x64 Checked gcstress0x3
test Ubuntu x64 Checked gcstress0xc
test Ubuntu x64 Checked gcstress0xc_jitstress1
test Ubuntu x64 Checked gcstress0xc_jitstress2
test Ubuntu x64 Checked gcstress0xc_minopts_heapverify1
test Ubuntu x64 Checked gcstress0xc_zapdisable
test Ubuntu x64 Checked gcstress0xc_zapdisable_heapverify1
test Ubuntu x64 Checked gcstress0xc_zapdisable_jitstress2

@4creators
Copy link
Copy Markdown

test Windows_NT x64 Checked jitincompletehwintrinsic
test Windows_NT x64 Checked jitx86hwintrinsicnoavx2
test Windows_NT x64 Checked jitx86hwintrinsicnosimd

test Windows_NT x86 Checked jitincompletehwintrinsic
test Windows_NT x86 Checked jitx86hwintrinsicnoavx2
test Windows_NT x86 Checked jitx86hwintrinsicnosimd

test Ubuntu x64 Checked jitincompletehwintrinsic
test Ubuntu x64 Checked jitx86hwintrinsicnoavx2
test Ubuntu x64 Checked jitx86hwintrinsicnosimd

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Wow, that is a lot of testing. Not sure we really needed all those GC stress legs, as this was an encoding assert that reproed deterministically, but I suppose it's harmless to run them again.

@fiigii
Copy link
Copy Markdown

fiigii commented Mar 22, 2018

@AndyAyersMS Thanks for the work!

@4creators we do not need to run the hwintrinsic tests, SSE2 is already fully implemented.

@4creators
Copy link
Copy Markdown

4creators commented Mar 22, 2018

we do not need to run the hwintrinsic tests, SSE2 is already fully implemented.

These are stress jobs with different configurations as default one which runs with AVX/AVX2 encoding.

@fiigii
Copy link
Copy Markdown

fiigii commented Mar 22, 2018

Two AVX/AVX2 tests failed with "GCStress=0xC + JitStress=1/2". I will look into them tomorrow.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

It occurs to me that we have no really effective way with dealing with this volume of tests when the baseline quality (especially for stress) is unclear and failures are either nondeterministic or known in baselines.

So I'm tempted to just ignore all the GC stress tests here as there is little reason to believe this change will impact them one way or another.

So far there are 3 non-GC stress failures

@dotnet-bot retest test Windows_NT x86 Checked jitincompletehwintrinsic

@AndyAyersMS
Copy link
Copy Markdown
Member Author

I can't repro the two x64 failures, and I don't see any diffs in the disassembly for pow_do or raytracer with complus_enableincompleteisaclass=1. So I can't explain why these failed.

@4creators
Copy link
Copy Markdown

AFAIR there are some SIMD tests which tend to fail sporadically but I have not seen pow_do or raytracer failing in this way before.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Continuing to categorize non-stress failures:

I sampled some of the failing GC stress legs and there weren't any new failures, just things that had also failed in a prior run. Since the CI system tracks failures on an order-run basis that's not definitive but it suggests (as we would suspect) that this change hasn't impacted GC stress itself one way or another.

So I will keep looking at the x64 cases locally and am going to give in and retry too.

@dotnet-bot retest test Windows_NT x64 Checked jitincompletehwintrinsic

@fiigii
Copy link
Copy Markdown

fiigii commented Mar 24, 2018

The failures of AVX/AVX2 ConvertToVector* look like managed code bugs (related to #16957 ?) because they even failed with disabled AVX/AVX2 intrinsics.

@tannergooding could you help?

@fiigii
Copy link
Copy Markdown

fiigii commented Mar 24, 2018

Detected a new bug from c488262, will be fixed by #17180

@AndyAyersMS
Copy link
Copy Markdown
Member Author

x64 passed on rerun. I think despite the large number of GC stress failures this is ready to merge. Those will have to be sorted separately.

@fiigii
Copy link
Copy Markdown

fiigii commented Mar 26, 2018

Can we merge this PR?

@AndyAyersMS
Copy link
Copy Markdown
Member Author

I think so, yeah. It fixes a deterministic assert and doesn't seem to have caused any regressions (to the best of my ability to tell, given the messy state of gc stress runs).

@dotnet/jit-contrib anyone disagree about merging this?

If I don't hear anything relatively soon, I'll go ahead and merge this.

@AndyAyersMS AndyAyersMS merged commit c7229d6 into dotnet:master Mar 26, 2018
@AndyAyersMS AndyAyersMS deleted the FixMovsd branch March 26, 2018 22:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants