Uh oh!
There was an error while loading. Please reload this page.
JIT: clear assertion input for unreachable blocks - #132898
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Fixes an unsound RangeCheck range-tightening behavior where encountering contradictory assertions could leave a previously-tightened (stale) range in place, enabling incorrect assertion-prop folding in optimized JIT/crossgen2 scenarios. Adds a targeted JIT regression test for the reported miscompile involving an enum-backed switch.
Changes:
- In
RangeCheck::MergeEdgeAssertionsWorker, when tightening yields an empty range, explicitly bail out by setting*pRangetoUnknowninstead of leaving the prior range intact. - Add a new JitBlue regression test (
Runtime_132879) exercising the miscompile shape underAggressiveOptimization. - Wire the new test into
Regression_ro_2.csproj.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/rangecheck.cpp | Makes contradictory assertion tightening conservative by resetting the output range to Unknown on empty intersections. |
| src/tests/JIT/Regression/JitBlue/Runtime_132879/Runtime_132879.cs | New xUnit regression test covering the switch/enum miscompile scenario. |
| src/tests/JIT/Regression/Regression_ro_2.csproj | Includes the new regression test in the merged test project. |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/coreclr/jit/rangecheck.cpp:1686
assertedRange.IsValid()bail-out returns without resetting*pRange. SinceassertedRangeis accumulated across assertions in this loop, it can become invalid when assertions contradict each other; returning here can still leak a partially-tightened (stale) range to the caller, similar to the empty-range case fixed below. To keep range inference sound in the presence of self-contradictory assertion sets, set*pRangeto Unknown before returning (same pattern as thecopy.IsValid()failure path).
JITDUMP("invalid range after tightening\n");
// The tightened range is empty, i.e. the assertions contradict the range we computed.
// If the assertion set really did hold here the block would be unreachable, but we get
// here with sets that do not hold at this point (e.g. assertions of a use block merged
// into the range of a definition that lives in another block). Returning while leaving
// the previously tightened range in place lets a fact that has just been disproven
// escape to the caller, which then folds branches with it. Bail out to Unknown instead.
*pRange = Range(Limit(Limit::keUnknown));
Uh oh!
There was an error while loading. Please reload this page.
Assertion dataflow initializes incoming sets to all assertions, but blocks outside the DFS tree are never visited and retain that lattice top. RBO can leave SSA definitions in those blocks reachable from range analysis, causing the uncomputed set to be treated as facts. Clear those incoming sets after dataflow so range analysis cannot consume them. Fixes#132879 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce554866-bacd-46ea-aac0-783dd4187ee0
09b28ff to
92f1a5aCompareUh oh!
There was an error while loading. Please reload this page.
Unreachable blocks also retain lattice-top outgoing assertion sets because dataflow never visits them. Clear both outgoing sets after dataflow so direct edge queries cannot treat the uncomputed sets as facts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce554866-bacd-46ea-aac0-783dd4187ee0
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
🟢 Approval recommended
The change is a small, targeted correctness fix that conservatively clears assertion facts only for non-DFS (unreached-by-dataflow) blocks, with low regression risk.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Assertion dataflow initializes every block's incoming and outgoing sets to all valid assertions, then computes the real sets only for blocks in the DFS tree. Blocks made unreachable by flow optimizations retain that initial lattice top.
RBO can leave an unreachable block in the block list with SSA definitions that range analysis still follows from reachable code. Assertion and range consumers could then read the block's uncomputed sets as if every tracked assertion held, producing an invalidly narrow range and folding the switch guard in #132879 incorrectly.
Clear
bbAssertionIn,bbAssertionOut, andbbJtrueAssertionOutfor blocks outside the DFS tree after assertion dataflow. This fixes the invariant at the producer and does not rely on finding contradictory assertions, so assertion-table budget overflow or unsupported assertion kinds cannot hide the problem. Reachable assertion sets and empty-range handling remain unchanged.The reported repro now completes 500,000/500,000 calls successfully. SuperPMI replay has no failures or asserts; aggregate asmdiffs remain 0.00% for both benchmarks.run and libraries.pmi.
Fixes#132879