Skip to content

Conversation

amanasifkhalid
Copy link
Contributor

Manual backport of #106765 to release/9.0

Customer Impact

  • Customer reported
  • Found internally

Internal fuzz testing exposed multiple cases (#106608, #106546, etc) where the usage of a hardware intrinsic with an out-of-range, non-const immediate would throw an exception in Debug builds, but not in Release builds. In Debug builds, if the immediate is non-const, the JIT would convert the intrinsic into a user call with a range check. In Release, the JIT might be able to convert the immediate into a constant via later optimizations like forward substitution, so it wouldn't pessimistically convert the intrinsic into a user call during importation. However, the JIT wouldn't always emit a range check to represent the exceptional path in IR, so even if IR rationalization would eventually convert an intrinsic with an unknown immediate into a user call, earlier optimizations could optimize away the intrinsic completely, hence the Debug/Release diffs.

In .NET 10, we would like to add fallback intrinsic implementations for cases where the immediate is out-of-range or non-const, not only to eliminate Debug/Release diffs, but to also improve the usability and performance of these intrinsics in non-const immediate scenarios. However, the scope of this work is too large to target .NET 9. For now, this PR adjusts the JIT's importation logic for HW intrinsics to always emit a range check for non-const immediates, and to always convert to a user call if the immediate is known to be out-of-range.

While working on this, we noticed the JIT was calculating the vector count for some variants of AdvSimd.StoreSelectedScalar incorrectly. The variants in question operate on ValueTuples of Vectors, rather than Vectors directly, so we had to add a special case to the importer to parse the intrinsic's SIMD size from the ValueTuple argument.

Regression

  • Yes
  • No

I'm not sure how long the potential for Debug/Release diffs for intrinsics with immediates has been possible. We found this problem after updating one of our fuzzing tools to generate tests with vectorized code. Since Roslyn already warns users to not use non-const immediates with intrinsics marked with ConstantExpected, the code pattern required to trigger this issue is explicitly discouraged, thus decreasing the odds of encountering this. However, AdvSimd.StoreSelectedScalar's SIMD size calculation was regressed during the .NET 9 cycle when its ValueTuple overloads were added in #93223.

Testing

This fix includes regression tests picked from the generated programs that exposed this issue.

Risk

Low. The StoreSelectedScalar fix is quite targeted; while there are other intrinsics with ValueTuple arguments, their SIMD sizes are hard-coded, so the JIT doesn't need to go down the newly-added ValueTuple path to parse the SIMD size at runtime. As for the range check fix, while this touches the common path for importing HW intrinsics across all supported platforms, the range check can be trivially optimized away by the JIT if the immediate is later known to be in-range, so code quality is not impacted. And as mentioned above, the usage of non-const immediates is likely uncommon in users' programs to begin with.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 27, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@amanasifkhalid
Copy link
Contributor Author

@tannergooding PTAL. I had to manually open this PR due to a merge conflict caused by the new flag in hwintrinsic.h, as the ordering of the flags in that file are slightly different between release/9.0 and main. My fix changes the value of the new flag to match the existing ordering, but this shouldn't impact functionality. Thanks!

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. please get a code review. once ready we can merge

@jeffschwMSFT jeffschwMSFT added the Servicing-approved Approved for servicing release label Aug 27, 2024
@amanasifkhalid
Copy link
Contributor Author

@jeffschwMSFT thank you for the review! Could you merge this when you have a moment, please?

@jeffschwMSFT jeffschwMSFT merged commit a92301b into dotnet:release/9.0 Aug 27, 2024
97 of 99 checks passed
@amanasifkhalid amanasifkhalid deleted the merge-106765 branch August 27, 2024 18:38
@github-actions github-actions bot locked and limited conversation to collaborators Sep 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants