Skip to content

Conversation

@github-actions
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
@dotnet-policy-service
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

@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
Contributor

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
Contributor

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

@amanasifkhalid
Copy link
Contributor

@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
Contributor

@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
Contributor

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
@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.

4 participants