JIT: Renable SSA-aware PHI jump threading#127103
JIT: Renable SSA-aware PHI jump threading#127103AndyAyersMS 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. Fix included here that was not in dotnet#126812: ensure that field uses of locals get the proper VN updates. Fixes dotnet#126976.
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR JIT redundant branch elimination (jump threading) to better handle SSA-aware PHI cases by allowing threading when PHI uses can be safely resolved and by keeping SSA/VN state consistent when rewriting those uses.
Changes:
- Adjust jump-thread viability checks to treat “global PHI uses” as potentially resolvable (rather than an immediate bail-out), enabling PHI-based threading in more cases.
- Update successor SSA uses during threading and fix VN rewriting for
GT_LCL_FLDso field loads get the correct derived field VN from the replacement SSA def.
| { | ||
| continue; | ||
| } | ||
| assert(oldSsaNum != phiUse.m_replacementSsaNum); |
There was a problem hiding this comment.
The new assert assumes the replacement SSA is always different from the current SSA. In some control-flow shapes (e.g., backedge PHI args that reuse the PHI SSA when the variable is not redefined on that edge), replacement can legitimately equal oldSsaNum. In that case we should treat this as a no-op and avoid calling AddUse again (which can double-count uses). Please restore the previous early-continue (or otherwise handle the equal case) instead of asserting.
| assert(oldSsaNum != phiUse.m_replacementSsaNum); | |
| if (oldSsaNum == phiUse.m_replacementSsaNum) | |
| { | |
| continue; | |
| } |
There was a problem hiding this comment.
We don't jump thread blocks with self-loops, so seeing the same SSA def here is unexpected.
| JITDUMP(FMT_BB " has global phi for V%02u.%u; no phi-based threading\n", block->bbNum, lclNum, | ||
| ssaNum); |
There was a problem hiding this comment.
This JITDUMP message says "no phi-based threading" but the code no longer returns here (it just sets hasGlobalPhiUses and later may still allow threading after optCanRewritePhiUses). Please update the message to reflect the new behavior (e.g., that global PHI uses require additional resolution) to avoid misleading diagnostics when debugging jump threading decisions.
| JITDUMP(FMT_BB " has global phi for V%02u.%u; no phi-based threading\n", block->bbNum, lclNum, | |
| ssaNum); | |
| JITDUMP(FMT_BB " has global phi for V%02u.%u; additional phi-use resolution may be required\n", | |
| block->bbNum, lclNum, ssaNum); |
There was a problem hiding this comment.
will fix this later, if there's no other feedback.
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.
Fix included here that was not in #126812: ensure that field uses of locals get the proper VN updates.
Fixes #126976.