Repository navigation
JIT: validate RBO relop rewrite operands - #133621
Conversation
In optRedundantDominatingBranch, ensure the remaining predicate can be rewritten safely. Fixes dotnet#133558. Co-authored-by: Copilot <[email protected]>
github-actions
Bot
added
the
area-CodeGen-coreclr
|
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 |
|
@EgorBo PTAL Fixes new opt in .NET 11. No diffs expected. |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes CoreCLR JIT branch-optimization correctness in a subtle VN-dependent area that warrants maintainer validation despite the targeted fix and added tests.
Pull request overview
This PR hardens the JIT’s redundant dominating branch optimization by ensuring it only rewrites a dominated relop when the VN relop operands correspond to the dominated compare’s actual tree operands, preventing incorrect simplifications when liberal VN “looks through” a materialized predicate. It also adds targeted JIT opt tests covering the materialized-predicate pattern in both relational directions.
Changes:
- Add an operand-VN equivalence check before rewriting the dominated relop operator during RBO simplification.
- Add new xUnit coverage for the “materialized
a != bpredicate + dominating relational compare” pattern for both<and>directions.
File summaries
| File | Description |
|---|---|
| src/coreclr/jit/redundantbranchopts.cpp | Adds a guard to prevent relop-only rewrites when VN operands don’t match the compare node’s operand VNs (avoids miscompiles with materialized predicates). |
| src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs | Adds regression tests that exercise the materialized-predicate scenario in both relational directions. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
I don't quite understand what you're suggesting. When going from VN space back to IR we have to be careful. Ideally this would be simpler and less error prone... |
|
/ba-g known mono test failures |
|
/backport to release/11.0 |
|
Started backporting to |
Backport of #133621 to release/11.0 /cc @AndyAyersMS ## Customer Impact - [ ] Customer reported - [x] Found internally The JIT can rewrite a materialized Boolean predicate using value-number operands that do not correspond to the actual tree operands, producing wrong code. The reported case returns `0` unconditionally when the correct result is `1`. This reproduces on .NET 11 RC2; .NET 8, 9, and 10 are correct. See #133558. ## Regression - [x] Yes - [ ] No Introduced during the .NET 11 cycle by the redundant-branch relop simplification added in #127181. ## Testing Added regression coverage for materialized predicates in both relational directions. All 35 `JIT.opt` `RedundantBranchSimplify` tests passed. Windows x64 Checked SuperPMI produced no assembly differences in 987,559 successful compilations, with 908 identical misses. ## Risk Low. The change adds a validation check before performing the existing rewrite. When the value-number operands do not match the concrete tree operands, the JIT conservatively skips the optimization. SuperPMI found no code-generation changes outside the new regression cases. Co-authored-by: Andy Ayers <[email protected]> Co-authored-by: Copilot <[email protected]>
Ensure dominating RBO only rewrites the remaining predicate when the VN relop operands match the concrete tree operands.
Add coverage for materialized predicates in both relational directions.
Fixes #133558.
Validation:
JIT.optRedundantBranchSimplify: 35 passedNote
This PR description was generated with GitHub Copilot CLI.