From b3fea64f95b8a8a41fa468f406199851975fb444 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 18 Sep 2026 09:15:46 -0700 Subject: [PATCH 1/2] JIT: Validate jump threading phi inputs Jump threading could remove a phi without accounting for all remaining predecessors, producing an invalid SSA rewrite. Require complete predecessor coverage before replacing phi uses. Fixes #133981 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 19e3976f-8f3e-4ab8-973d-d568620770c1 --- src/coreclr/jit/redundantbranchopts.cpp | 25 +++++++++++-- .../JIT/opt/RedundantBranch/JumpThreadPhi.cs | 37 +++++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 3fde336a8efead..bb073bba62f6a1 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1585,6 +1585,7 @@ static bool optGetThreadedSsaNumForBlock(JumpThreadInfo& jti, GenTreeLclVar* phi assert(jti.m_numAmbiguousPreds != 0); bool foundReplacement = false; + int numCoveredPreds = 0; unsigned replacementSsa = SsaConfig::RESERVED_SSA_NUM; GenTreePhi* const phi = phiDef->Data()->AsPhi(); @@ -1598,6 +1599,8 @@ static bool optGetThreadedSsaNumForBlock(JumpThreadInfo& jti, GenTreeLclVar* phi continue; } + numCoveredPreds++; + if (!foundReplacement) { replacementSsa = phiArgNode->GetSsaNum(); @@ -1609,7 +1612,7 @@ static bool optGetThreadedSsaNumForBlock(JumpThreadInfo& jti, GenTreeLclVar* phi } } - if (!foundReplacement) + if (!foundReplacement || (numCoveredPreds != jti.m_numAmbiguousPreds)) { return false; } @@ -1643,7 +1646,21 @@ static bool optGetThreadedSsaNumForSuccessor(JumpThreadInfo& jti, *hasThreadedPreds = false; *replacementSsaNum = SsaConfig::RESERVED_SSA_NUM; + int numThreadedPreds = 0; + if (jti.m_trueTarget == successor) + { + numThreadedPreds += jti.m_numTruePreds; + } + if (jti.m_falseTarget == successor) + { + numThreadedPreds += jti.m_numFalsePreds; + } + + *hasThreadedPreds = numThreadedPreds != 0; + int const numExpectedPreds = jti.m_numAmbiguousPreds + numThreadedPreds; + bool foundReplacement = false; + int numCoveredPreds = 0; unsigned replacementSsa = SsaConfig::RESERVED_SSA_NUM; GenTreePhi* const phi = phiDef->Data()->AsPhi(); @@ -1661,10 +1678,10 @@ static bool optGetThreadedSsaNumForSuccessor(JumpThreadInfo& jti, { continue; } - - *hasThreadedPreds = true; } + numCoveredPreds++; + if (!foundReplacement) { replacementSsa = phiArgNode->GetSsaNum(); @@ -1677,7 +1694,7 @@ static bool optGetThreadedSsaNumForSuccessor(JumpThreadInfo& jti, } *replacementSsaNum = replacementSsa; - return foundReplacement; + return foundReplacement && (numCoveredPreds == numExpectedPreds); } //------------------------------------------------------------------------ diff --git a/src/tests/JIT/opt/RedundantBranch/JumpThreadPhi.cs b/src/tests/JIT/opt/RedundantBranch/JumpThreadPhi.cs index 64fa4824bd23f9..71c27d77d5970b 100644 --- a/src/tests/JIT/opt/RedundantBranch/JumpThreadPhi.cs +++ b/src/tests/JIT/opt/RedundantBranch/JumpThreadPhi.cs @@ -28,4 +28,41 @@ public static void TestPhi00() Assert.Equal(100, Phi_00(1)); Assert.Equal(103, Phi_00(8)); } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static int Phi_01(bool q, bool r, int u, int v) + { + int z = 0, w = 0, m = 0, n = 0; + if (r) + { + m = v; + n = 1; + goto Target; + } + + if (q) + { + z = u; + w = 1; + } + + if (z != w) + { + return -1; + } + + Target: + if (m == n) + { + return 200; + } + + return 100 + n; + } + + [Fact] + public static void TestPhi01() + { + Assert.Equal(200, Phi_01(false, false, 0, 0)); + } } From 6892e97f98bd26dd6352248b59f559782a880a66 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 18 Sep 2026 16:53:39 -0700 Subject: [PATCH 2/2] Harden jump threading PHI coverage Compare predecessor identities when validating threaded PHI replacements. This prevents stale PHI arguments from masking missing current predecessors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 19e3976f-8f3e-4ab8-973d-d568620770c1 --- src/coreclr/jit/redundantbranchopts.cpp | 56 ++++++++++++++----------- 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index bb073bba62f6a1..bcf34fdb97af77 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1585,7 +1585,7 @@ static bool optGetThreadedSsaNumForBlock(JumpThreadInfo& jti, GenTreeLclVar* phi assert(jti.m_numAmbiguousPreds != 0); bool foundReplacement = false; - int numCoveredPreds = 0; + BitVec coveredPreds = BitVecOps::MakeEmpty(&jti.traits); unsigned replacementSsa = SsaConfig::RESERVED_SSA_NUM; GenTreePhi* const phi = phiDef->Data()->AsPhi(); @@ -1599,7 +1599,7 @@ static bool optGetThreadedSsaNumForBlock(JumpThreadInfo& jti, GenTreeLclVar* phi continue; } - numCoveredPreds++; + BitVecOps::AddElemD(&jti.traits, coveredPreds, predBlock->bbPostorderNum); if (!foundReplacement) { @@ -1612,7 +1612,7 @@ static bool optGetThreadedSsaNumForBlock(JumpThreadInfo& jti, GenTreeLclVar* phi } } - if (!foundReplacement || (numCoveredPreds != jti.m_numAmbiguousPreds)) + if (!foundReplacement || !BitVecOps::Equal(&jti.traits, coveredPreds, jti.m_ambiguousPreds)) { return false; } @@ -1646,21 +1646,34 @@ static bool optGetThreadedSsaNumForSuccessor(JumpThreadInfo& jti, *hasThreadedPreds = false; *replacementSsaNum = SsaConfig::RESERVED_SSA_NUM; - int numThreadedPreds = 0; - if (jti.m_trueTarget == successor) + BitVec expectedPreds = BitVecOps::MakeCopy(&jti.traits, jti.m_ambiguousPreds); + for (BasicBlock* const predBlock : jti.m_block->PredBlocks()) { - numThreadedPreds += jti.m_numTruePreds; - } - if (jti.m_falseTarget == successor) - { - numThreadedPreds += jti.m_numFalsePreds; - } + if (BitVecOps::IsMember(&jti.traits, jti.m_ambiguousPreds, predBlock->bbPostorderNum)) + { + continue; + } - *hasThreadedPreds = numThreadedPreds != 0; - int const numExpectedPreds = jti.m_numAmbiguousPreds + numThreadedPreds; + BasicBlock* predTarget = nullptr; + if (BitVecOps::IsMember(&jti.traits, jti.m_truePreds, predBlock->bbPostorderNum)) + { + predTarget = jti.m_trueTarget; + } + else + { + assert(jti.m_numFalsePreds != 0); + predTarget = jti.m_falseTarget; + } + + if (predTarget == successor) + { + BitVecOps::AddElemD(&jti.traits, expectedPreds, predBlock->bbPostorderNum); + *hasThreadedPreds = true; + } + } bool foundReplacement = false; - int numCoveredPreds = 0; + BitVec coveredPreds = BitVecOps::MakeEmpty(&jti.traits); unsigned replacementSsa = SsaConfig::RESERVED_SSA_NUM; GenTreePhi* const phi = phiDef->Data()->AsPhi(); @@ -1668,19 +1681,12 @@ static bool optGetThreadedSsaNumForSuccessor(JumpThreadInfo& jti, { 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) + if (!BitVecOps::IsMember(&jti.traits, expectedPreds, predBlock->bbPostorderNum)) { - BasicBlock* const predTarget = isTruePred ? jti.m_trueTarget : jti.m_falseTarget; - if (predTarget != successor) - { - continue; - } + continue; } - numCoveredPreds++; + BitVecOps::AddElemD(&jti.traits, coveredPreds, predBlock->bbPostorderNum); if (!foundReplacement) { @@ -1694,7 +1700,7 @@ static bool optGetThreadedSsaNumForSuccessor(JumpThreadInfo& jti, } *replacementSsaNum = replacementSsa; - return foundReplacement && (numCoveredPreds == numExpectedPreds); + return foundReplacement && BitVecOps::Equal(&jti.traits, coveredPreds, expectedPreds); } //------------------------------------------------------------------------