JIT: fix widened IV init placed in unreachable block#130190
Open
EgorBo wants to merge 1 commit into
Open
Conversation
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
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
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.
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: