Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -7839,7 +7839,8 @@ class Compiler
bool optRedundantBranch(BasicBlock* const block);
bool optJumpThreadDom(BasicBlock* const block, BasicBlock* const domBlock, bool domIsSameRelop);
bool optJumpThreadPhi(BasicBlock* const block, GenTree* tree, ValueNum treeNormVN);
bool optJumpThreadCheck(BasicBlock* const block, BasicBlock* const domBlock);
bool optJumpThreadCheck(BasicBlock* const block, BasicBlock* const domBlock,
bool* pHasGlobalPhiUse = nullptr);
bool optJumpThreadCore(JumpThreadInfo& jti);

enum class ReachabilityResult
Expand Down
66 changes: 51 additions & 15 deletions src/coreclr/jit/redundantbranchopts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1055,13 +1055,17 @@ struct JumpThreadInfo
// optJumpThreadCheck: see if block is suitable for jump threading.
//
// Arguments:
// block - block in question
// domBlock - dom block used in inferencing (if any)
// block - block in question
// domBlock - dom block used in inferencing (if any)
// 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
Comment on lines +1060 to +1062
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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

Copilot uses AI. Check for mistakes.
// predecessor is fully classified).
//
// Returns:
// True if the block is suitable for jump threading.
//
bool Compiler::optJumpThreadCheck(BasicBlock* const block, BasicBlock* const domBlock)
bool Compiler::optJumpThreadCheck(BasicBlock* const block, BasicBlock* const domBlock, bool* pHasGlobalPhiUse)
{
// If the block is the first block of try-region, then skip jump threading
if (bbIsTryBeg(block))
Expand Down Expand Up @@ -1121,27 +1125,42 @@ bool Compiler::optJumpThreadCheck(BasicBlock* const block, BasicBlock* const dom
unsigned const ssaNum = phiDef->GetSsaNum();
LclVarDsc* const varDsc = lvaGetDesc(lclNum);

bool isProblematicPhi = false;

// We do not put implicit uses of promoted local fields into SSA.
// So assume the worst here, that there is some implicit use of this ssa
// def we don't know about.
//
if (varDsc->lvIsStructField)
{
JITDUMP(FMT_BB " has phi for promoted field V%02u.%u; no phi-based threading\n", block->bbNum,
lclNum, ssaNum);
return false;
isProblematicPhi = true;
}
else
{
LclSsaVarDsc* const ssaVarDsc = varDsc->GetPerSsaData(ssaNum);

LclSsaVarDsc* const ssaVarDsc = varDsc->GetPerSsaData(ssaNum);
// Bypassing a global use might require SSA updates.
// Note a phi use is ok if it's local (self loop)
//
if (ssaVarDsc->HasGlobalUse())
{
isProblematicPhi = true;
}
}

// Bypassing a global use might require SSA updates.
// Note a phi use is ok if it's local (self loop)
//
if (ssaVarDsc->HasGlobalUse())
if (isProblematicPhi)
{
JITDUMP(FMT_BB " has global phi for V%02u.%u; no phi-based threading\n", block->bbNum, lclNum,
ssaNum);
return false;
if (pHasGlobalPhiUse != nullptr)
{
// Caller will decide whether to bail based on pred classification.
*pHasGlobalPhiUse = true;
}
else
{
JITDUMP(FMT_BB " has global/promoted phi for V%02u.%u; no phi-based threading\n",
block->bbNum, lclNum, ssaNum);
return false;
}
}
}

Expand Down Expand Up @@ -1395,8 +1414,11 @@ bool Compiler::optJumpThreadDom(BasicBlock* const block, BasicBlock* const domBl
bool Compiler::optJumpThreadPhi(BasicBlock* block, GenTree* tree, ValueNum treeNormVN)
{
// 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)
Comment on lines 1416 to 1422
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
{
return false;
Expand Down Expand Up @@ -1625,6 +1647,20 @@ bool Compiler::optJumpThreadPhi(BasicBlock* block, GenTree* tree, ValueNum treeN
}
}

// 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;
}
Comment on lines +1650 to +1662
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// Do the optimization.
//
return optJumpThreadCore(jti);
Expand Down
Loading