From 49b6df92b333111fd06cfe54964c04a67e05f090 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 19 Sep 2026 08:48:06 -0700 Subject: [PATCH] JIT: Validate jump threading phi inputs (#134205) Jump threading can see an incomplete phi after an earlier flow edit. The SSA replacement logic only checked the phi arguments that remained, so it could remove the phi and rewrite uses without accounting for every predecessor. Require the block and successor replacement paths to cover every expected predecessor before accepting a common SSA definition. Otherwise, conservatively skip the rewrite. Add a regression case to `JumpThreadPhi` that returned 101 instead of 200 before the fix. Validation: - Windows x64 Checked JIT build and matching Core_Root - `JumpThreadPhi`: 2 passed; new test fails before the fix and passes after it - Standalone repro: 101 before, 200 after - JIT formatting - Windows x64 Release SuperPMI: 0 failures; 11 stable textual diffs across the four affected collections, +82 bytes over about 1.86 million sequentially replayed contexts Resolves dotnet/runtime#133981 > [!NOTE] > This pull request description was generated with GitHub Copilot. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 19e3976f-8f3e-4ab8-973d-d568620770c1 --- src/coreclr/jit/redundantbranchopts.cpp | 49 ++++++++++++++----- .../JIT/opt/RedundantBranch/JumpThreadPhi.cs | 37 ++++++++++++++ 2 files changed, 73 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 2cfd29ebba7b29..8481a1344a2ac1 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1581,6 +1581,7 @@ static bool optGetThreadedSsaNumForBlock(JumpThreadInfo& jti, GenTreeLclVar* phi assert(jti.m_numAmbiguousPreds != 0); bool foundReplacement = false; + BitVec coveredPreds = BitVecOps::MakeEmpty(&jti.traits); unsigned replacementSsa = SsaConfig::RESERVED_SSA_NUM; GenTreePhi* const phi = phiDef->Data()->AsPhi(); @@ -1594,6 +1595,8 @@ static bool optGetThreadedSsaNumForBlock(JumpThreadInfo& jti, GenTreeLclVar* phi continue; } + BitVecOps::AddElemD(&jti.traits, coveredPreds, predBlock->bbPostorderNum); + if (!foundReplacement) { replacementSsa = phiArgNode->GetSsaNum(); @@ -1605,7 +1608,7 @@ static bool optGetThreadedSsaNumForBlock(JumpThreadInfo& jti, GenTreeLclVar* phi } } - if (!foundReplacement) + if (!foundReplacement || !BitVecOps::Equal(&jti.traits, coveredPreds, jti.m_ambiguousPreds)) { return false; } @@ -1639,7 +1642,34 @@ static bool optGetThreadedSsaNumForSuccessor(JumpThreadInfo& jti, *hasThreadedPreds = false; *replacementSsaNum = SsaConfig::RESERVED_SSA_NUM; + BitVec expectedPreds = BitVecOps::MakeCopy(&jti.traits, jti.m_ambiguousPreds); + for (BasicBlock* const predBlock : jti.m_block->PredBlocks()) + { + if (BitVecOps::IsMember(&jti.traits, jti.m_ambiguousPreds, predBlock->bbPostorderNum)) + { + continue; + } + + 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; + BitVec coveredPreds = BitVecOps::MakeEmpty(&jti.traits); unsigned replacementSsa = SsaConfig::RESERVED_SSA_NUM; GenTreePhi* const phi = phiDef->Data()->AsPhi(); @@ -1647,20 +1677,13 @@ 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; - } - - *hasThreadedPreds = true; + continue; } + BitVecOps::AddElemD(&jti.traits, coveredPreds, predBlock->bbPostorderNum); + if (!foundReplacement) { replacementSsa = phiArgNode->GetSsaNum(); @@ -1673,7 +1696,7 @@ static bool optGetThreadedSsaNumForSuccessor(JumpThreadInfo& jti, } *replacementSsaNum = replacementSsa; - return foundReplacement; + return foundReplacement && BitVecOps::Equal(&jti.traits, coveredPreds, expectedPreds); } //------------------------------------------------------------------------ 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)); + } }