Repository navigation
JIT: fix switch peeling stealing the wrong operand of its compare - #132395
Conversation
|
Azure Pipelines: Successfully started running 8 pipeline(s). 8 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
3eedfbe to
de3ab29
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a JIT correctness issue in fgPeelSwitch where operand swapping during statement sequencing could cause the peeled switch to dispatch on the wrong value, and adds a new Arm64 LSE atomics “light-up” path via a new Lse intrinsic plus associated JIT/AOT plumbing. Also introduces a JIT regression test for the switch-peeling miscompile. (Not built/tested as part of this review.)
Changes:
- JIT: build the dominant-case compare fully before statement creation in
fgPeelSwitchto avoid operand swapping changing which operand is “stolen”. - Arm64 atomics: introduce
System.Runtime.Intrinsics.Arm.Lseand route Interlocked atomic ops throughif (Lse.IsSupported)where applicable; update JIT importer/lowering/LSRA/codegen and AOT/R2R metadata/tooling to support it. - Tests: add
Runtime_132370regression test reproducing the miscompile scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/Regression/JitBlue/Runtime_132370/Runtime_132370.cs | Adds regression coverage for the switch-peeling/unroll miscompile. |
| src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs | Adds Arm64 Lse.IsSupported fast-paths for And/Or on word-sized integers. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Lse.cs | Introduces new internal Arm64 LSE intrinsic surface used by Interlocked. |
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Includes the new Lse.cs source file in CoreLib builds. |
| src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetDesc.txt | Names the ARM64 atomics instruction set implementation type as Lse. |
| src/coreclr/tools/Common/JitInterface/CorInfoInstructionSet.cs | Maps System.Runtime.Intrinsics.Arm.Lse to ARM64_Atomics and roots the type. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunHardwareIntrinsicRootProvider.cs | Avoids rooting generic intrinsic method definitions for R2R compilation. |
| src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs | Adds Arm64 Lse.IsSupported light-up for key atomic Interlocked APIs (CoreCLR flavor). |
| src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs | Adds Arm64 Lse.IsSupported light-up for key atomic Interlocked APIs (NativeAOT flavor). |
| src/coreclr/jit/lsraarm64.cpp | Uses per-node atomics implementation choice to adjust internal regs / lifetimes. |
| src/coreclr/jit/lowerarmarch.cpp | Disables contained immediates for atomic ops when emitting LSE forms. |
| src/coreclr/jit/jitconfigvalues.h | Adds Arm64 stress config for exercising managed atomics light-up. |
| src/coreclr/jit/importercalls.cpp | Leaves select Interlocked calls unexpanded in the “dynamic” atomics mode to enable managed light-up. |
| src/coreclr/jit/hwintrinsiclistarm64.h | Adds Arm64 Atomics intrinsic definitions (CompareAndSwap/LoadAdd/LoadClear/LoadSet/Swap). |
| src/coreclr/jit/hwintrinsicarm64.cpp | Maps Atomics intrinsics to existing atomic IR nodes and enforces type constraints. |
| src/coreclr/jit/hwintrinsic.cpp | Registers Atomics intrinsic range so they’re recognized as a real ISA group. |
| src/coreclr/jit/gentree.h | Adds GTF_ATOMIC_LSE flag to distinguish LSE-vs-LL/SC expansion. |
| src/coreclr/jit/gentree.cpp | Prevents CSE/merging of atomic nodes that differ by GTF_ATOMIC_LSE. |
| src/coreclr/jit/fgopt.cpp | Fixes fgPeelSwitch sequencing so the peeled compare steals the correct operand. |
| src/coreclr/jit/compiler.h | Introduces AtomicsImpl + per-node selection helper for atomic expansion strategy. |
| src/coreclr/jit/compiler.cpp | Implements cached atomics expansion mode selection (Lse/LlSc/Dynamic). |
| src/coreclr/jit/codegenarm64.cpp | Uses per-node atomics expansion decision to emit LSE instructions vs LL/SC loops. |
fgPeelSwitch creates "EQ(switchValue, dominantCase)" for the peeled check, puts
it into a statement and only then reattaches the switch value to the switch via
fgMakeMultiUse(&compare->gtOp1).
Creating the statement sequences the tree, and gtSetEvalOrder is free to swap the
operands of a compare - it does exactly that when op1 is a constant ("encourage
constants to be evaluated last"). The switch value is a constant whenever the
loop containing the switch was unrolled, so the switch ended up dispatching on
the dominant case constant instead of on its own value: every unrolled copy of
the switch jumped to the case 0 target, silently producing wrong results.
Build the compare completely (including the multi-use and the flag updates)
before handing it to fgNewStmtFromTree.
Fixes dotnet#132370
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bd309226-4fb2-49aa-8586-b3dad59f3a36
de3ab29 to
dc550f6
Compare
|
PTAL, user-reported bug, small fix, no diffs @jakobbotsch @dotnet/jit-contrib |
|
/ba-g infra issues |
…its compare (#132631) Backport of #132395 to release/10.0 ## Customer Impact - [x] Customer reported - [ ] Found internally Reported in #132370, with a standalone 71-line repro. Silent wrong code on a default installation — no configuration or environment variables needed. `fgPeelSwitch` reattached the switch value via `fgMakeMultiUse(&compare->gtOp1)` *after* the compare was put into a statement. Creating the statement sequences the tree via `gtSetEvalOrder`, which swaps a compare's operands when op1 is a constant, so the switch ended up "stealing" the dominant case constant instead of the switch value. The switch value is a constant when the loop around the switch is unrolled, so every unrolled copy dispatched on case 0. In the reported case a method checking whether all three components of a vector are zero returned `true` for `(0, 0, 1)`. Fix: build the compare fully before creating the statement. ## Regression - [x] Yes - [ ] No .NET 10. The reporter verified every published .NET 10 runtime is affected (all 21, from preview.1 through 10.0.11, 10/10 runs each); .NET 8 and .NET 9 are not. ## Testing Regression test `Runtime_132370` from the original PR is included. The original PR reported no asm diffs (benchmarks.run, libraries.pmi). ## Risk Low. Pure ordering change within `fgPeelSwitch` — the compare is now fully built before it is sequenced, so the operand it hands to the switch is the intended one. No behavior change beyond that. ## Notes on the backport Nearly clean; `fgopt.cpp` applies verbatim. Only the test wiring differs: `main` registers the test in the merged runner `src/tests/JIT/Regression/Regression_ro_2.csproj`, which does not exist on release/10.0, so the test gets its own `Runtime_132370.csproj` per this branch's JitBlue convention. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Copilot-Session: ec11ff08-3e62-4386-a86e-51f868acffe1
|
/backport to release/11.0 |
|
Started backporting to |
|
@AndyAyersMS backporting to git am output$ git cherry-pick 915f2aee2b74485bda851be24b41c6a263e95d96
Auto-merging src/coreclr/jit/fgopt.cpp
Auto-merging src/tests/JIT/Regression/Regression_ro_2.csproj
CONFLICT (content): Merge conflict in src/tests/JIT/Regression/Regression_ro_2.csproj
error: could not apply 915f2aee2b7... JIT: fix switch peeling stealing the wrong operand of its compare (#132395)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: JIT: fix switch peeling stealing the wrong operand of its compare
Using index info to reconstruct a base tree...
M src/tests/JIT/Regression/Regression_ro_2.csproj
Falling back to patching base and 3-way merge...
Auto-merging src/tests/JIT/Regression/Regression_ro_2.csproj
CONFLICT (content): Merge conflict in src/tests/JIT/Regression/Regression_ro_2.csproj
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 JIT: fix switch peeling stealing the wrong operand of its compare
Error: The process '/usr/bin/git' failed with exit code 128 |
fgPeelSwitchreattached the switch value viafgMakeMultiUse(&compare->gtOp1)after the compare was put into a statement. Sequencing runsgtSetEvalOrder, which swaps a compare's operands when op1 is a constant, so the switch got the dominant case constant instead of the switch value.The switch value is a constant when the loop around the switch is unrolled, so each unrolled copy dispatched on case 0 -> wrong results.
Fix: build the compare fully before creating the statement.
No asm diffs (benchmarks.run, libraries.pmi).
Fixes #132370