[release/8.0-staging] Ensure that constant folding for SIMD shifts on xarch follow the correct behavior on overshift #98066
Conversation
…ect behavior on overshift (dotnet#98001) * Ensure that constant folding for SIMD shifts on xarch follow the correct behavior on overshift * Ensure we test Sse2.IsSupported
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsBackport of #98001 to release/8.0-staging /cc @tannergooding Customer ImpactThis was a customer reported issue and was a regression from .NET 7. The impact is that users may see incorrect results in release code. AnalysisSupport for constant folding some primitive SIMD intrinsics was added in .NET 8. For the most part, the behavior of these intrinsics is consistent across platforms and consistent with the underlying scalar APIs users will typically consume from C#. However, "overshifting" is not well-defined behavior and can differ across platforms. Overshifting, in particular, is when you provide a shift amount more than the number of bits available in the underlying data type. For example, given IL leaves this as strictly undefined behavior and leaves it to the underlying platform, while C# makes it well defined and prescribes that it is actually While we have tests covering overshifting, we missed a test covering the combination of overshifting by a constant amount and thus the edge case that can occur in some kinds of user code. TestingA regression test covering the scenario was added as well as other potentially interesting combinations. RiskLow. The issue is well understood, the other constant folding support added has been checked for potential edge cases, and the functionality has also been checked against Arm64.
|
|
CC. @EgorBo, could you provide sign-off on this as well so it can get marked |
|
I thought that was a theoretical corner-case, does it meet the bar for backporting? |
|
While it is effectively an edge case in an already niche scenario, it has been hit by at least two independent users in unrelated workloads that I'm aware of. Its notably also a net new regression, in a pre-existing API in a recently released LTS, so I believe it meets all the qualifications making it appropriate to backport. |
tannergooding
added
the
Servicing-consider
jeffschwMSFT
left a comment
jeffschwMSFT
left a comment
There was a problem hiding this comment.
approved. we will take for consideration in 8.0.x
rbhanda
added
Servicing-approved
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Failures are for |
|
CC. @EgorBo, @dotnet/jit-contrib for merging. Monday February 12th is the Code Complete deadline for the March Release, so it needs to be merged by then (Julie asked that I let one of the JIT reviewers do the actual merge here). The extra-platform failures are unrelated and occuring in regular outerloop (had run them to validate the same bug wasn't there on Mono). |
4 participants
Backport of #98001 to release/8.0-staging
/cc @tannergooding
Customer Impact
Developers who are shifting by a constant amount while using the platform specific APIs may see incorrect results.
Regression
Support for constant folding some primitive SIMD intrinsics was added in .NET 8. For the most part, the behavior of these intrinsics is consistent across platforms and consistent with the underlying scalar APIs users will typically consume from C#. However, "overshifting" is not well-defined behavior and can differ across platforms. Overshifting, in particular, is when you provide a shift amount more than the number of bits available in the underlying data type. For example, given
int x, doingx << 32is not "well-defined" across languages and platforms.IL leaves this as strictly undefined behavior and leaves it to the underlying platform, while C# makes it well defined and prescribes that it is actually
x << (32 % (sizeof(int) * 8)), which is equivalent to it wrapping around and thus is the same asx << 0. This matches what quite a bit of hardware does, such asx86/x64for their scalar shift instructions. However, the vector shift instructions for x86/x64 differ and instead treat it as actually shifting by 1, 32 times. Thus it produces a value that is all zero.While we have tests covering overshifting, we missed a test covering the combination of overshifting by a constant amount and thus the edge case that can occur in some kinds of user code.
Testing
A regression test covering the scenario was added as well as other potentially interesting combinations.
Risk
Low. The issue is well understood, the other constant folding support added has been checked for potential edge cases, and the functionality has also been checked against Arm64.