JIT: Prevent loop hoisting across volatile reads - #133825
Merged
AndyAyersMS merged 2 commits intoSep 15, 2026
Merged
Conversation
Volatile reads have acquire semantics, but loop optimization could hoist an ordinary load across a prior iteration's acquire. Treat volatile indirections as loop memory havoc and add a regression test for the cross-iteration ordering. Fixes dotnet#133579 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Member
Author
|
@jakobbotsch another one for you to review Should be minimal diff |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The regression test can miss the unsafe hoisting without tighter synchronization.
Pull request overview
Prevents JIT loop hoisting across volatile reads by modeling volatile indirections as loop memory havoc.
Changes:
- Updates loop side-effect analysis for volatile reads.
- Adds a cross-thread regression test for acquire ordering.
File summaries
| File | Description |
|---|---|
src/coreclr/jit/optimizer.cpp |
Models volatile indirections as loop memory effects. |
src/tests/JIT/Regression_o_2/Runtime_40444.cs |
Adds regression coverage; synchronization should occur after the first load. |
Review details
Suppressed comments (1)
src/tests/JIT/Regression_o_2/Runtime_40444.cs:128
- The event is signaled before the first
box.Valueload, so it does not establish that the load the optimizer may place in the loop preheader has already executed. If the reader is descheduled afterSet()and before that load, the writer can publish42and the unpatched JIT will also pass this test, leaving the regression uncovered. Signal immediately after the first load inside the loop so the writer cannot race past the load being tested.
readerStarted.Set();
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
jakobbotsch
reviewed
Sep 14, 2026
Name the test after issue 133579 and mark it as an outer-loop test so it runs at priority 1 without changing the priority of the merged regression assembly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 51ab0cbf-6d32-4701-8e9d-fa19e4d01f6f
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The regression test needs deterministic synchronization to reliably expose the unfixed behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
jakobbotsch
approved these changes
Sep 15, 2026
jtschuster
pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 18, 2026
Volatile reads have acquire semantics, but loop optimization could hoist an ordinary load across a prior iteration's acquire. Treat volatile indirections as loop memory havoc and add a regression test for the cross-iteration ordering. Fixes dotnet#133579 Validation: - Verified the regression test fails without the fix and passes with it. - Verified the existing Runtime_40444 regression still passes. - Built Checked and Release CoreCLR and ran JIT formatting. - Ran Windows x64 Checked SuperPMI diffs across nine collections: 2 of 987,559 successful compilations changed, totaling +4 bytes and effectively 0.0000% aggregate PerfScore impact. > [!NOTE] > This pull request description was generated with GitHub Copilot. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 51ab0cbf-6d32-4701-8e9d-fa19e4d01f6f
Member
Author
|
/backport to release/11.0 |
Contributor
|
Started backporting to |
4 tasks
JulieLeeMSFT
pushed a commit
that referenced
this pull request
Sep 21, 2026
) Backport of #133825 to release/11.0 /cc @AndyAyersMS ## Customer Impact - [ ] Customer reported - [x] Found internally Loop optimization can hoist an ordinary field load across a `Volatile.Read` acquire from a prior loop iteration, violating the managed memory model and allowing stale data to be observed. The reported repro returns `0` after another thread publishes `42`. This affects .NET 8, 9, 10, and 11. A community contributor noted that application code may encounter this silently and that the cause would be difficult to diagnose. See #133579. ## Regression - [ ] Yes - [x] No No recent introducing change has been identified. The issue reproduces across all currently supported runtime versions, including .NET 8 through .NET 11. ## Testing Added a focused cross-iteration ordering regression test that fails without the fix and passes with it. The existing Runtime_40444 volatile-ordering regression also passes. Validation included Checked and Release CoreCLR builds, JIT formatting, and Windows x64 Checked SuperPMI across nine collections. Only 2 of 987,559 successful compilations changed, totaling +4 bytes with effectively zero aggregate PerfScore impact. ## Risk Low. The change treats volatile indirections as loop memory havoc, conservatively preventing unsafe load hoisting across acquire operations. It may inhibit optimization in the affected shape, but SuperPMI measured only two tiny code-generation changes and no meaningful performance impact. --------- Co-authored-by: Andy Ayers <andya@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 51ab0cbf-6d32-4701-8e9d-fa19e4d01f6f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Volatile reads have acquire semantics, but loop optimization could hoist an
ordinary load across a prior iteration's acquire. Treat volatile indirections as
loop memory havoc and add a regression test for the cross-iteration ordering.
Fixes #133579
Validation:
successful compilations changed, totaling +4 bytes and effectively 0.0000%
aggregate PerfScore impact.
Note
This pull request description was generated with GitHub Copilot.