JIT: fix widened IV init placed in unreachable block#130190
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR hardens redundant-branch optimization (RBO) dominator-based jump threading to avoid leaving SSA in an invalid state when threading bypasses blocks that contain PHI definitions with non-local (global) uses. It aligns the dominator-based path with the existing PHI-based threading safety checks by requiring PHI-use resolution (via optCanRewritePhiUses) or bailing out when resolution isn’t safe, and it adds a regression test for the reported miscompile.
Changes:
- Extend
optJumpThreadCheckto detect globally-used local PHI defs for both dom-based and PHI-based threading, and treat promoted-field PHIs as non-threadable. - Update dominator-based jump threading (
optJumpThreadDom) to invokeoptCanRewritePhiUseswhen global PHI uses are detected, and skip jump threading if uses can’t be safely rewritten. - Add a new JitBlue regression test (C# + csproj) validating the previously incorrect result.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/coreclr/jit/redundantbranchopts.cpp |
Adds PHI global-use detection to jump-thread eligibility checks and gates dominator-based threading on optCanRewritePhiUses when PHI-use rewriting is required. |
src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.cs |
Adds an xUnit regression test reproducing the miscompile and asserting the correct result. |
src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.csproj |
Adds the test project with optimization enabled and tiered compilation disabled for determinism. |
4ecb3a9 to
77795a9
Compare
optWidenPrimaryIV initializes the widened IV in the block that holds the narrow IV's reaching def when that is not the preheader. Redundant branch opts jump threading can leave that def block unreachable while its SSA def still reaches the loop (stale SSA). The init was then emitted into dead code that later gets removed, leaving the widened IV uninitialized and producing a wrong result. Only use the reaching-def block for the init when it is still reachable (present in the DFS tree); otherwise fall back to the preheader. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
77795a9 to
c17d3c3
Compare
|
PTAL @jakobbotsch @dotnet/jit-contrib An issue reported internally. I tried to bail on it in RBO, but that led to terrible diffs |
Fixes #130189.
optWidenPrimaryIVinitializes the widened IV in the block that holds the narrow IV's reaching def when that block is not the preheader. Redundant branch opts jump threading can bypass that def block and leave it unreachable while its SSA def still reaches the loop (stale SSA). The widened IV init was then emitted into that dead block, which a later flow-graph pass removes, so the widened IV was never initialized and the loop produced a wrong result.Fix: only use the reaching-def block for the init when it is still reachable (present in the DFS tree). Otherwise fall back to the preheader, which is always reachable.
Notes: