Ensure VN handles both forms of the xarch shift instructions for SIMD #91601
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis resolves #91175
|
src/tests/JIT/Regression/JitBlue/Runtime_91175/Runtime_91175.cs
Outdated
Show resolved
Hide resolved
| [Fact] | ||
| public static void TestEntryPoint() | ||
| { | ||
| _ = Method0(); |
There was a problem hiding this comment.
The test is sufficient to cover the reported issue. However, it would be nice that the test checks numerical correctness as well.
There was a problem hiding this comment.
We have a number of other tests covering numerical correctness of the functionality.
This is namely a regression test covering a very specific assert.
There was a problem hiding this comment.
Had there was a numerical correctness test case covering this specific case, it would had failed on CI before this fix, right? I don't see this PR enabling any existing test. Thus, I suspect that this is sufficient numerical correctness test coverage for the scenario that this PR is fixing.
There was a problem hiding this comment.
We have tests validating it here (which covers overshifting both via constant and non-constant): https://github.com/dotnet/runtime/blob/main/src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_X86.cs#L347C1-L348
This pr is fixing an edge case around value numbering in a very particular scenario and ensuring we don't assert. We were already effectively doing the right thing codegen wise.
There was a problem hiding this comment.
I assume those tests aren't covering the VN constant folding, or we would have found this issue and #88451 as well. Can we somehow validate basic correctness of the rest of the constant folding that was added in .NET 8?
This pr is fixing an edge case around value numbering in a very particular scenario and ensuring we don't assert. We were already effectively doing the right thing codegen wise.
The release behavior here is to hit a noway assert, so I think we should backport this fix to .NET 8.
There was a problem hiding this comment.
Let's consider adding more testing separately from this so we can get going on the backport.
|
@tannergooding @jakobbotsch This PR was approved. Can it be merged? Also, @jakobbotsch suggested back-porting to .NET 8. Can we do that before RC2? |
I don't think we need this in RC2 since it's not customer reported, so we have some time to get the GA backport in. |
ghost
locked as resolved and limited conversation to collaborators
This resolves #91175