Uh oh!
There was an error while loading. Please reload this page.
JumpThreading: don't give up on phis in non-ambiguous preds - #126711
JumpThreading: don't give up on phis in non-ambiguous preds#126711EgorBo wants to merge 1 commit into
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Updates CoreCLR JIT redundant-branch/jump-threading logic to avoid prematurely bailing out of PHI-based threading when PHI “problem” cases exist but all predecessors can be fully classified, addressing the missed optimization opportunity described in #126703.
Changes:
- Extend
optJumpThreadCheckwith an optional out-parameter to report (instead of immediately rejecting) problematic PHIs during PHI-based jump threading. - In
optJumpThreadPhi, defer the “problematic PHI” bail-out until after predecessor classification and only bail when ambiguous predecessors remain. - Update the JIT compiler interface in
compiler.hto match the newoptJumpThreadChecksignature.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/coreclr/jit/redundantbranchopts.cpp | Adds deferred handling for problematic PHIs during PHI-based jump threading; bails only when ambiguous preds remain. |
| src/coreclr/jit/compiler.h | Updates optJumpThreadCheck declaration to include an optional out-parameter. |
| // pHasGlobalPhiUse - optional output; when non-null, global phi uses | ||
| // are reported here instead of causing an immediate bail-out, | ||
| // letting the caller decide (e.g. allow threading when every |
There was a problem hiding this comment.
The pHasGlobalPhiUse out-parameter is also set for promoted-field PHIs (lvIsStructField), not just “global phi uses” as the name/docstring implies. Consider renaming this to something like pHasProblematicPhi (or splitting promoted-field vs global-use flags) and updating the comment so callers don’t misinterpret what it indicates.
| // pHasGlobalPhiUse - optional output; when non-null, global phi uses | |
| // are reported here instead of causing an immediate bail-out, | |
| // letting the caller decide (e.g. allow threading when every | |
| // pHasGlobalPhiUse - optional output; when non-null, PHI cases that are | |
| // problematic for jump threading are reported here instead | |
| // of causing an immediate bail-out. This currently includes | |
| // global phi uses and promoted-field PHIs, letting the | |
| // caller decide (e.g. allow threading when every |
| // First see if block is eligible for threading. | ||
| // Defer the global-phi bail-out: when every predecessor is fully classified | ||
| // the block becomes unreachable after threading, so global phi uses are safe. | ||
| // | ||
| const bool check = optJumpThreadCheck(block, /* domBlock*/ nullptr); | ||
| bool hasGlobalPhiUse = false; | ||
| const bool check = optJumpThreadCheck(block, /* domBlock*/ nullptr, &hasGlobalPhiUse); | ||
| if (!check) |
There was a problem hiding this comment.
hasGlobalPhiUse is used as a general “problematic PHI present” indicator (it’s set for promoted-field PHIs too), but the surrounding comment and later bail-out message talk only about “global phi”. Please align the naming/messages with the actual semantics (e.g., mention promoted-field PHIs here as well, or rename the flag).
| // If there are ambiguous predecessors and a global phi use, we cannot | ||
| // safely thread because the block would remain reachable (from the | ||
| // ambiguous preds) with a broken PHI. | ||
| // | ||
| // When there are NO ambiguous preds, all preds are redirected, | ||
| // the block becomes unreachable, and global phi uses are harmless. | ||
| // | ||
| if (hasGlobalPhiUse && (jti.m_numAmbiguousPreds > 0)) | ||
| { | ||
| JITDUMP(FMT_BB " has global phi and %u ambiguous pred(s); no phi-based threading\n", | ||
| block->bbNum, jti.m_numAmbiguousPreds); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This change is intended to unlock a specific codegen improvement (issue #126703), but there’s no regression test exercising the TrailingZeroCount(x) + sentinel compare pattern. Consider adding a JIT disasm-check test (see existing HasDisasmCheck tests under src/tests/JIT/opt/Compares/) that asserts the extra compare/cmove is eliminated for the idx != -1 case.
AndyAyersMS
commented
Apr 9, 2026
EgorBo
commented
Apr 9, 2026
AndyAyersMS
commented
Apr 9, 2026
We may be able to do something like the following:
This will catch the example here as there is just one global use, it's in one of the successors, and that successor is join-free. If we can't account for local vs global uses accurately, we would also need to scan the jump thread block and count up its uses. |
testing a fix for #126703