Repository navigation
JIT: Do not contain local addresses of volatile indirections on xarch - #135720
Conversation
A volatile access to an address-exposed local was emitted through the emitter stack move path, where IsRedundantStackMov has no notion of volatility and dropped an identical consecutive store (or a load right after a store). Keep the local address in a register for volatile indirections so these accesses are never elided. Rare in practice: +21 bytes over 4 methods in libraries.pmi. Fixes dotnet#135386 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88502efb-96e2-4483-b698-59f5003a2e2c
|
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. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
🟡 Changes recommended
The regression test passes with the faulty code and must verify the emitted volatile accesses.
1 open finding
What changed in this PR
Prevents xarch JIT peephole optimization from eliding volatile accesses to address-exposed locals.
Changes:
- Keeps volatile local addresses in registers.
- Adds a regression test, though it cannot currently detect the bug.
| File | Description |
|---|---|
src/coreclr/jit/lowerxarch.cpp |
Disables local-address containment for volatile indirections. |
src/tests/JIT/Regression_ro_2/Runtime_135386.cs |
Adds coverage for consecutive volatile stores. |
🧠 Review effort: Balanced
|
PTAL @dotnet/jit-contrib, trivial bug-fix |
JulieLeeMSFT
left a comment
There was a problem hiding this comment.
LGTM. It prevents the JIT from incorrectly eliminating consecutive identical volatile accesses to address-exposed locals, preserving required memory ordering and visibility.
tannergooding
left a comment
There was a problem hiding this comment.
LGTM. It might be good if we had a way to rationalize such loads/stores to emit so we didn't have to pessimize codegen. This feels like something we could handle via the additional flags we support passing down to emi.
A volatile access to an address-exposed local was emitted through the emitter stack move path, where IsRedundantStackMov has no notion of volatility and dropped an identical consecutive store (or a load right after a store). Keep the local address in a register for volatile indirections so these accesses are never elided. Rare in practice: +21 bytes over 4 methods in libraries.pmi.
Fixes #135386
A few regressions (required for correctness).