Skip to content

[release/9.0] JIT: Cast UInt64 to Single directly during const folding #106720

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Aug 21, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 20, 2024

Backport of #106419 to release/9.0

/cc @amanasifkhalid

Customer Impact

  • Customer reported
  • Found internally

#106338 exposed different cast behavior between ulong and float on ARM64 (and x64 with AVX-512) in Debug and Release builds. In the unoptimized case, we emit code to do the cast, and ARM64 supports casting directly from 64-bit integers to float without an intermediate cast to double. When optimizing, we evaluate the cast at compile-time, but the JIT's constant folding logic does the cast in two steps (i.e. ulong -> double -> float), which as demonstrated by the above issue, can result in subtle rounding differences. Our toolchain's compilers (MSVC/Clang/GCC/etc.) all emit the correct instruction sequence for ulong -> float casts, so the simplest fix for Debug/Release diffs is to adjust the constant-folding logic to cast directly to float.

While this fixes Debug/Release diffs, it's worth noting that the JIT still performs two-step casts on some platforms (x86, x64 without AVX-512, among others). This has to do partly with the fact that Roslyn emits a two-opcode IL sequence for ulong -> float casts, and RyuJIT imports this IL as a ulong -> double -> float cast; later on, RyuJIT may or may not decide to morph this into a ulong -> float cast, depending on the target architecture. We plan to fix this for all platforms such that an intermediate cast is never introduced (and if we import a two-step cast, we don't incorrectly morph it into one cast), but this work would likely be too disruptive to include in .NET 9 at this point. We're soliciting discussion on a solution for this in #106646.

Regression

  • Yes
  • No

Testing

This issue was exposed when one of our fuzz testers generated a case that exercises subtle floating-point semantics. My fix includes a regression test to check the casting behavior we currently expect across our supported platforms. Once #106646 is fixed, we can expect the casting behavior this test demonstrates to be unified across platforms.

Risk

Low. This fix is quite surgical in removing Debug/Release diffs on platforms RyuJIT currently supports ulong -> float casts for.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 20, 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
Member

@tannergooding PTAL, thank you!

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. we can merge when ready

@amanasifkhalid
Copy link
Member

I opened #106731 to track the build failures on arm64, but Build Analysis is stuck, despite not listing any unknown issues (link).

@amanasifkhalid
Copy link
Member

/ba-g Build Analysis is red, despite all issues being known

@amanasifkhalid
Copy link
Member

@jeffschwMSFT @carlossanlop could you merge this when you have a moment? I'm not authorized to merge to release/9.0. Thanks!

@jeffschwMSFT
Copy link
Member

@amanasifkhalid can you update the branch as it is out of date?

@amanasifkhalid
Copy link
Member

@jeffschwMSFT @carlossanlop does this PR need anything else, or can it be merged?

@jeffschwMSFT
Copy link
Member

looks like it still needs a branch update. with that it can be merged

@amanasifkhalid
Copy link
Member

looks like it still needs a branch update. with that it can be merged

Done

@jeffschwMSFT jeffschwMSFT merged commit 06269ca into release/9.0 Aug 21, 2024
10 of 15 checks passed
@amanasifkhalid amanasifkhalid deleted the backport/pr-106419-to-release/9.0 branch August 21, 2024 17:07
@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 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