JIT: support SSA-aware PHI jump threading#126812
JIT: support SSA-aware PHI jump threading#126812AndyAyersMS wants to merge 1 commit intodotnet:mainfrom
Conversation
Teach redundant branch elimination to keep jump threading through PHI- based blocks when the PHI uses can be fully accounted for in the block and its immediate successors. Rewrite the affected successor SSA/VN uses, keep dominating-block threading conservative, and add focused regression coverage for the new PHI-based cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR extends CoreCLR JIT redundant branch optimization (jump threading) to support PHI-based threading when PHI definitions have non-local uses, by accounting for those uses within the threaded block and its immediate successors and rewriting successor SSA/VN uses accordingly. It also adds a focused JIT regression test to cover the new PHI-based scenario.
Changes:
- Add tracking of PHI SSA uses in the candidate block/successors and rewrite successor SSA/VN uses when threading bypasses the PHI block.
- Refine jump-thread eligibility checking to return a richer classification (including “needs PHI use resolution”).
- Add a new JIT test case (
JumpThreadPhi) to validate the new behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/redundantbranchopts.cpp | Implements PHI-use discovery and successor SSA/VN rewriting; updates jump-thread check flow. |
| src/coreclr/jit/compiler.h | Introduces JumpThreadCheckResult and declares new helper methods for PHI-use analysis/rewriting. |
| src/coreclr/jit/compmemkind.h | Adds a dedicated allocator kind for redundant-branch/jump-threading data. |
| src/tests/JIT/opt/RedundantBranch/JumpThreadPhi.cs | Adds regression coverage for PHI-based jump threading through a PHI-producing pattern. |
| src/tests/JIT/opt/RedundantBranch/JumpThreadPhi.csproj | Adds the test project definition consistent with existing tests in the folder. |
| // | ||
| // Returns: | ||
| // True if the block is suitable for jump threading. | ||
| // Viability of jump threading: either CannotThread, CanThread, or CanThreadWithPhiBasedRBO. |
There was a problem hiding this comment.
The comment for optJumpThreadCheck’s return value list is out of date: it mentions “CanThreadWithPhiBasedRBO”, but the actual result type is JumpThreadCheckResult with values CannotThread/CanThread/NeedsPhiUseResolution. Please update the comment to match the enum to avoid confusion for future maintainers.
| // Viability of jump threading: either CannotThread, CanThread, or CanThreadWithPhiBasedRBO. | |
| // Viability of jump threading: either CannotThread, CanThread, or NeedsPhiUseResolution. |
| for (GenTreePhi::Use& use : phi->Uses()) | ||
| { | ||
| GenTreePhiArg* const phiArgNode = use.GetNode()->AsPhiArg(); | ||
| BasicBlock* const predBlock = phiArgNode->gtPredBB; | ||
| bool const isTruePred = BitVecOps::IsMember(&jti.traits, jti.m_truePreds, predBlock->bbPostorderNum); | ||
| bool const isAmbiguousPred = BitVecOps::IsMember(&jti.traits, jti.m_ambiguousPreds, predBlock->bbPostorderNum); | ||
|
|
||
| if (isAmbiguousPred) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| BasicBlock* const predTarget = isTruePred ? jti.m_trueTarget : jti.m_falseTarget; | ||
| if (predTarget != successor) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| *hasThreadedPreds = true; | ||
|
|
||
| if (!foundReplacement) | ||
| { | ||
| replacementSsa = phiArgNode->GetSsaNum(); | ||
| foundReplacement = true; | ||
| } | ||
| else if (replacementSsa != phiArgNode->GetSsaNum()) | ||
| { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
optGetThreadedSsaNumForSuccessor does not verify that every predecessor that will be redirected to the given successor has a corresponding PhiArg entry in the PHI. If SSA has been made stale (which this file already notes can happen), missing PhiArgs would be silently ignored and the function could incorrectly report a unique replacement SSA. Consider explicitly validating coverage for each (non-ambiguous) pred that targets this successor and failing the rewrite if any pred is not represented in the PHI args.
| unsigned* replacementSsaNum) | ||
| { | ||
| assert(successor != nullptr); | ||
|
|
There was a problem hiding this comment.
optGetThreadedSsaNumForSuccessor relies on the caller to initialize the out parameter *hasThreadedPreds before the call. Since this is an [OUT] parameter, it would be safer/clearer to set *hasThreadedPreds = false (and *replacementSsaNum = RESERVED) at function entry so the helper can’t be misused accidentally later.
| *hasThreadedPreds = false; | |
| *replacementSsaNum = SsaConfig::RESERVED_SSA_NUM; |
|
Follow-on to #126711. If all the uses of a global phi in a jump-threading candidate block are in the block and its successors, we can (in most cases) allow jump threading and rewrite the successor uses to the appropriate new ssa def & vn. We avoid cases where the successors already have PHIs or would need to have new PHIs. These seem somewhat rare. @EgorBo PTAL |
Teach redundant branch elimination to keep jump threading through PHI- based blocks when the PHI uses can be fully accounted for in the block and its immediate successors. Rewrite the affected successor SSA/VN uses, keep dominating-block threading conservative, and add focused regression coverage for the new PHI-based cases.
Fixes #126703(not quite, yet)