From 69f8460e6e09a369db9c884780b459345c6cf568 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 12 Jul 2024 16:55:14 +0200 Subject: [PATCH 1/5] JIT: Avoid strength reducing IVs that are live out of a loop These primary IVs are not going to be removed by the downwards loop transformation, so if we do this we introduce additional primary IVs which (for now) it is the goal not to. --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/inductionvariableopts.cpp | 86 +++++++++++++++-------- 2 files changed, 58 insertions(+), 29 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 72d89347b3eed7..395efc71812b56 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7616,6 +7616,7 @@ class Compiler FlowGraphNaturalLoop* loop, BasicBlock* exiting, LoopLocalOccurrences* loopLocals); + bool optPrimaryIVHasNonLoopUses(unsigned lclNum, FlowGraphNaturalLoop* loop, LoopLocalOccurrences* loopLocals); bool optWidenPrimaryIV(FlowGraphNaturalLoop* loop, unsigned lclNum, ScevAddRec* addRec, diff --git a/src/coreclr/jit/inductionvariableopts.cpp b/src/coreclr/jit/inductionvariableopts.cpp index bb4131a5c3b4c1..6166273ba7a901 100644 --- a/src/coreclr/jit/inductionvariableopts.cpp +++ b/src/coreclr/jit/inductionvariableopts.cpp @@ -1012,34 +1012,10 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte break; } - unsigned candidateLclNum = stmt->GetRootNode()->AsLclVarCommon()->GetLclNum(); - LclVarDsc* candidateVarDsc = lvaGetDesc(candidateLclNum); - if (candidateVarDsc->lvIsStructField && loopLocals->HasAnyOccurrences(loop, candidateVarDsc->lvParentLcl)) - { - continue; - } + unsigned candidateLclNum = stmt->GetRootNode()->AsLclVarCommon()->GetLclNum(); - if (candidateVarDsc->lvDoNotEnregister) + if (!optPrimaryIVHasNonLoopUses(candidateLclNum, loop, loopLocals)) { - // This filters out locals that may be live into exceptional exits. - continue; - } - - BasicBlockVisit visitResult = loop->VisitRegularExitBlocks([=](BasicBlock* block) { - if (VarSetOps::IsMember(this, block->bbLiveIn, candidateVarDsc->lvVarIndex)) - { - return BasicBlockVisit::Abort; - } - - return BasicBlockVisit::Continue; - }); - - if (visitResult == BasicBlockVisit::Abort) - { - // Live into an exit. - // TODO-CQ: In some cases it may be profitable to materialize the final value after the loop. - // This requires analysis on whether the required expressions are available there - // (and whether it doesn't extend their lifetimes too much). continue; } @@ -1099,9 +1075,6 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte return false; } - // At this point we know that the single exit dominates all backedges. - JITDUMP(" All backedges are dominated by exiting block " FMT_BB "\n", exiting->bbNum); - if (loop->MayExecuteBlockMultipleTimesPerIteration(exiting)) { JITDUMP(" Exiting block may be executed multiple times per iteration; cannot place decrement in it\n"); @@ -1193,6 +1166,54 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte return true; } +//------------------------------------------------------------------------ +// optPrimaryIVIsLoopScoped: +// Check if a primary IV may have uses outside the specified loop. +// +// Parameters: +// lclNum - The primary IV +// loop - The loop +// loopLocals - Data structure tracking local uses +// +// Returns: +// True if the primary IV may have non-loop uses (or if it is a field with +// uses of the parent struct). +// +bool Compiler::optPrimaryIVHasNonLoopUses(unsigned lclNum, FlowGraphNaturalLoop* loop, LoopLocalOccurrences* loopLocals) +{ + LclVarDsc* varDsc = lvaGetDesc(lclNum); + if (varDsc->lvIsStructField && loopLocals->HasAnyOccurrences(loop, varDsc->lvParentLcl)) + { + return false; + } + + if (varDsc->lvDoNotEnregister) + { + // This filters out locals that may be live into exceptional exits. + return false; + } + + BasicBlockVisit visitResult = loop->VisitRegularExitBlocks([=](BasicBlock* block) { + if (VarSetOps::IsMember(this, block->bbLiveIn, varDsc->lvVarIndex)) + { + return BasicBlockVisit::Abort; + } + + return BasicBlockVisit::Continue; + }); + + if (visitResult == BasicBlockVisit::Abort) + { + // Live into an exit. + // TODO-CQ: In some cases it may be profitable to materialize the final value after the loop. + // This requires analysis on whether the required expressions are available there + // (and whether it doesn't extend their lifetimes too much). + return false; + } + + return true; +} + struct CursorInfo { BasicBlock* Block; @@ -1319,6 +1340,13 @@ bool StrengthReductionContext::TryStrengthReduce() continue; } + if (m_comp->optPrimaryIVHasNonLoopUses(primaryIVLcl->GetLclNum(), m_loop, &m_loopLocals)) + { + // We won't be able to remove this primary IV + JITDUMP(" Has non-loop uses\n"); + continue; + } + ScevAddRec* primaryIV = static_cast(candidate); InitializeCursors(primaryIVLcl, primaryIV); From 8cc20671bcc4f4c46fc91943198d04f5328c8798 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 17 Jul 2024 14:15:38 +0200 Subject: [PATCH 2/5] Expand to complete the checks --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/inductionvariableopts.cpp | 230 ++++++++++++---------- 2 files changed, 132 insertions(+), 99 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 395efc71812b56..e20d095f2e1a28 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7616,6 +7616,7 @@ class Compiler FlowGraphNaturalLoop* loop, BasicBlock* exiting, LoopLocalOccurrences* loopLocals); + bool optCanAndShouldChangeExitTest(GenTree* cond, bool dump); bool optPrimaryIVHasNonLoopUses(unsigned lclNum, FlowGraphNaturalLoop* loop, LoopLocalOccurrences* loopLocals); bool optWidenPrimaryIV(FlowGraphNaturalLoop* loop, unsigned lclNum, diff --git a/src/coreclr/jit/inductionvariableopts.cpp b/src/coreclr/jit/inductionvariableopts.cpp index 6166273ba7a901..7b1d4020f12c6b 100644 --- a/src/coreclr/jit/inductionvariableopts.cpp +++ b/src/coreclr/jit/inductionvariableopts.cpp @@ -975,6 +975,9 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte BasicBlock* exiting, LoopLocalOccurrences* loopLocals) { + // Note: keep the heuristics here in sync with + // `StrengthReductionContext::IsUseExpectedToBeRemoved`. + assert(exiting->KindIs(BBJ_COND)); Statement* jtrueStmt = exiting->lastStmt(); @@ -982,21 +985,8 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte assert(jtrue->OperIs(GT_JTRUE)); GenTree* cond = jtrue->gtGetOp1(); - if ((jtrue->gtFlags & GTF_SIDE_EFFECT) != 0) - { - // If the IV is used as part of the side effect then we can't - // transform; otherwise we could. TODO-CQ: Make this determination and - // extract side effects from the jtrue to make this work. - JITDUMP(" No; exit node has side effects\n"); - return false; - } - - bool checkProfitability = !compStressCompile(STRESS_DOWNWARDS_COUNTED_LOOPS, 50); - - if (checkProfitability && cond->OperIsCompare() && - (cond->gtGetOp1()->IsIntegralConst(0) || cond->gtGetOp2()->IsIntegralConst(0))) + if (!optCanAndShouldChangeExitTest(cond, /* dump */ true)) { - JITDUMP(" No; operand of condition [%06u] is already 0\n", dspTreeID(cond)); return false; } @@ -1014,17 +1004,18 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte unsigned candidateLclNum = stmt->GetRootNode()->AsLclVarCommon()->GetLclNum(); - if (!optPrimaryIVHasNonLoopUses(candidateLclNum, loop, loopLocals)) + if (optPrimaryIVHasNonLoopUses(candidateLclNum, loop, loopLocals)) { continue; } bool hasUseInTest = false; - auto checkRemovableUse = [=, &hasUseInTest](BasicBlock* block, Statement* stmt) { + auto checkRemovableUse = [=, &hasUseInTest](BasicBlock* block, Statement* stmt, GenTree* tree) { if (stmt == jtrueStmt) { hasUseInTest = true; - // Use is inside the loop test that has no side effects (as we checked above), can remove + // Use is inside the loop test that we know we can change (from + // calling optCanChangeExitTest above) return true; } @@ -1053,7 +1044,7 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte return true; }; - if (!loopLocals->VisitStatementsWithOccurrences(loop, candidateLclNum, checkRemovableUse)) + if (!loopLocals->VisitOccurrences(loop, candidateLclNum, checkRemovableUse)) { // Aborted means we found a non-removable use continue; @@ -1069,6 +1060,7 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte removableLocals.Push(candidateLclNum); } + bool checkProfitability = !compStressCompile(STRESS_DOWNWARDS_COUNTED_LOOPS, 50); if (checkProfitability && (removableLocals.Height() <= 0)) { JITDUMP(" Found no potentially removable locals when making this loop downwards counted\n"); @@ -1166,9 +1158,36 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte return true; } +bool Compiler::optCanAndShouldChangeExitTest(GenTree* cond, bool dump) +{ + if ((cond->gtFlags & GTF_SIDE_EFFECT) != 0) + { + // This would be possible if the IV use is not part of the side effect, + // in which case we could extract them. However, these cases turn out + // to be never analyzable for us even if we tried to do that, so just + // do the easy check here. + if (dump) + JITDUMP(" No; exit node has side effects\n"); + return false; + } + + bool checkProfitability = !compStressCompile(STRESS_DOWNWARDS_COUNTED_LOOPS, 50); + + if (checkProfitability && cond->OperIsCompare() && + (cond->gtGetOp1()->IsIntegralConst(0) || cond->gtGetOp2()->IsIntegralConst(0))) + { + if (dump) + JITDUMP(" No; operand of condition [%06u] is already 0\n", dspTreeID(cond)); + return false; + } + + return true; +} + //------------------------------------------------------------------------ // optPrimaryIVIsLoopScoped: -// Check if a primary IV may have uses outside the specified loop. +// Check if a primary IV may have uses of the primary IV that we do not +// reason about. // // Parameters: // lclNum - The primary IV @@ -1184,13 +1203,13 @@ bool Compiler::optPrimaryIVHasNonLoopUses(unsigned lclNum, FlowGraphNaturalLoop* LclVarDsc* varDsc = lvaGetDesc(lclNum); if (varDsc->lvIsStructField && loopLocals->HasAnyOccurrences(loop, varDsc->lvParentLcl)) { - return false; + return true; } if (varDsc->lvDoNotEnregister) { // This filters out locals that may be live into exceptional exits. - return false; + return true; } BasicBlockVisit visitResult = loop->VisitRegularExitBlocks([=](BasicBlock* block) { @@ -1208,10 +1227,10 @@ bool Compiler::optPrimaryIVHasNonLoopUses(unsigned lclNum, FlowGraphNaturalLoop* // TODO-CQ: In some cases it may be profitable to materialize the final value after the loop. // This requires analysis on whether the required expressions are available there // (and whether it doesn't extend their lifetimes too much). - return false; + return true; } - return true; + return false; } struct CursorInfo @@ -1220,14 +1239,12 @@ struct CursorInfo Statement* Stmt; GenTree* Tree; ScevAddRec* IV; - bool IsInsideExitTest = false; - CursorInfo(BasicBlock* block, Statement* stmt, GenTree* tree, ScevAddRec* iv, bool isInsideExitTest) + CursorInfo(BasicBlock* block, Statement* stmt, GenTree* tree, ScevAddRec* iv) : Block(block) , Stmt(stmt) , Tree(tree) , IV(iv) - , IsInsideExitTest(isInsideExitTest) { } }; @@ -1246,6 +1263,7 @@ class StrengthReductionContext void InitializeSimplificationAssumptions(); bool InitializeCursors(GenTreeLclVarCommon* primaryIVLcl, ScevAddRec* primaryIV); + bool IsUseExpectedToBeRemoved(BasicBlock* block, Statement* stmt, GenTreeLclVarCommon* tree); void AdvanceCursors(ArrayStack* cursors, ArrayStack* nextCursors); bool CheckAdvancedCursors(ArrayStack* cursors, int derivedLevel, ScevAddRec** nextIV); bool TryReplaceUsesWithNewPrimaryIV(ArrayStack* cursors, ScevAddRec* iv); @@ -1479,16 +1497,11 @@ bool StrengthReductionContext::InitializeCursors(GenTreeLclVarCommon* primaryIVL m_cursors2.Reset(); auto visitor = [=](BasicBlock* block, Statement* stmt, GenTreeLclVarCommon* tree) { - if (stmt->GetRootNode()->OperIsLocalStore()) + if (IsUseExpectedToBeRemoved(block, stmt, tree)) { - GenTreeLclVarCommon* lcl = stmt->GetRootNode()->AsLclVarCommon(); - if ((lcl->GetLclNum() == primaryIVLcl->GetLclNum()) && ((lcl->Data()->gtFlags & GTF_SIDE_EFFECT) == 0)) - { - // Store to the primary IV without side effects; if we end - // up strength reducing, then this store is expected to be - // removed by making the loop downwards counted. - return true; - } + // If we do strength reduction we expect to be able to remove this + // use; do not create a cursor for it. + return true; } if (!tree->OperIs(GT_LCL_VAR)) @@ -1496,16 +1509,12 @@ bool StrengthReductionContext::InitializeCursors(GenTreeLclVarCommon* primaryIVL return false; } - bool isInsideExitTest = - block->KindIs(BBJ_COND) && (stmt == block->lastStmt()) && - (!m_loop->ContainsBlock(block->GetTrueTarget()) || !m_loop->ContainsBlock(block->GetFalseTarget())); - if (tree->GetSsaNum() != primaryIVLcl->GetSsaNum()) { // Most likely a post-incremented use of the primary IV; we // could replace these as well, but currently we only handle // the cases where we expect the use to be removed. - return isInsideExitTest; + return false; } Scev* iv = m_scevContext.Analyze(block, tree); @@ -1522,8 +1531,8 @@ bool StrengthReductionContext::InitializeCursors(GenTreeLclVarCommon* primaryIVL // as the primary IV. assert(Scev::Equals(m_scevContext.Simplify(iv, m_simplAssumptions), primaryIV)); - m_cursors1.Emplace(block, stmt, tree, primaryIV, isInsideExitTest); - m_cursors2.Emplace(block, stmt, tree, primaryIV, isInsideExitTest); + m_cursors1.Emplace(block, stmt, tree, primaryIV); + m_cursors2.Emplace(block, stmt, tree, primaryIV); return true; }; @@ -1541,8 +1550,7 @@ bool StrengthReductionContext::InitializeCursors(GenTreeLclVarCommon* primaryIVL for (int i = 0; i < m_cursors1.Height(); i++) { CursorInfo& cursor = m_cursors1.BottomRef(i); - printf(" [%d] [%06u]%s: ", i, Compiler::dspTreeID(cursor.Tree), - cursor.IsInsideExitTest ? " (in-test)" : ""); + printf(" [%d] [%06u]: ", i, Compiler::dspTreeID(cursor.Tree)); cursor.IV->Dump(m_comp); printf("\n"); } @@ -1552,6 +1560,83 @@ bool StrengthReductionContext::InitializeCursors(GenTreeLclVarCommon* primaryIVL return true; } +//------------------------------------------------------------------------ +// IsUseExpectedToBeRemoved: Check if a use of a primary IV is expected to be +// removed if we strength reduce other uses of the primary IV. +// +// Parameters: +// block - Block containing the use +// stmt - Statement containing the use +// tree - Actual use of the primary IV +// +// Returns: +// True if the use is expected to be removable. +// +bool StrengthReductionContext::IsUseExpectedToBeRemoved(BasicBlock* block, Statement* stmt, GenTreeLclVarCommon* tree) +{ + unsigned primaryIVLclNum = tree->GetLclNum(); + if (stmt->GetRootNode()->OperIsLocalStore()) + { + GenTreeLclVarCommon* lcl = stmt->GetRootNode()->AsLclVarCommon(); + if ((lcl->GetLclNum() == primaryIVLclNum) && ((lcl->Data()->gtFlags & GTF_SIDE_EFFECT) == 0)) + { + // Store to the primary IV without side effects; if we end + // up strength reducing, then this store is expected to be + // removed by making the loop downwards counted. + return true; + } + + return false; + } + + bool isInsideExitTest = + block->KindIs(BBJ_COND) && (stmt == block->lastStmt()) && + (!m_loop->ContainsBlock(block->GetTrueTarget()) || !m_loop->ContainsBlock(block->GetFalseTarget())); + + if (isInsideExitTest) + { + // The downwards loop transformation may be able to remove this use. + // Here we duplicate some of the logic from + // optMakeExitTestDownwardsCounted to predict whether that will happen. + GenTree* jtrue = block->lastStmt()->GetRootNode(); + GenTree* cond = jtrue->gtGetOp1(); + + // Is the exit test changeable? + if (!m_comp->optCanAndShouldChangeExitTest(cond, /* dump */ false)) + { + return false; + } + + // Does the exit dominate all backedges such that we can place IV + // updates before it? + for (FlowEdge* edge : m_loop->BackEdges()) + { + if (!m_comp->m_domTree->Dominates(block, edge->getSourceBlock())) + { + return false; + } + } + + // Will the exit only run once per iteration? + if (m_loop->MayExecuteBlockMultipleTimesPerIteration(block)) + { + return false; + } + + // Can we compute the trip count from the exit test? + if (m_scevContext.ComputeExitNotTakenCount(block) == nullptr) + { + return false; + } + + // If all of those things are true, we are most likely going to be able + // to convert the exit test to a down-counting one after we have removed the other uses of the IV. + return true; + } + + return false; +} + //------------------------------------------------------------------------ // AdvanceCursors: Advance cursors stored in "cursors" and store the advanced // result in "nextCursors". @@ -1569,8 +1654,7 @@ void StrengthReductionContext::AdvanceCursors(ArrayStack* cursors, A CursorInfo& cursor = cursors->BottomRef(i); CursorInfo& nextCursor = nextCursors->BottomRef(i); - assert((nextCursor.Block == cursor.Block) && (nextCursor.Stmt == cursor.Stmt) && - (nextCursor.IsInsideExitTest == cursor.IsInsideExitTest)); + assert((nextCursor.Block == cursor.Block) && (nextCursor.Stmt == cursor.Stmt)); nextCursor.Tree = cursor.Tree; do @@ -1613,8 +1697,7 @@ void StrengthReductionContext::AdvanceCursors(ArrayStack* cursors, A for (int i = 0; i < nextCursors->Height(); i++) { CursorInfo& nextCursor = nextCursors->BottomRef(i); - printf(" [%d] [%06u]%s: ", i, nextCursor.Tree == nullptr ? 0 : Compiler::dspTreeID(nextCursor.Tree), - nextCursor.IsInsideExitTest ? " (in-test)" : ""); + printf(" [%d] [%06u]%s: ", i, nextCursor.Tree == nullptr ? 0 : Compiler::dspTreeID(nextCursor.Tree)); if (nextCursor.IV == nullptr) { printf(""); @@ -1658,13 +1741,6 @@ bool StrengthReductionContext::CheckAdvancedCursors(ArrayStack* curs { CursorInfo& cursor = cursors->BottomRef(i); - // Uses inside the exit test only need to opportunistically - // match. We check these after. - if (cursor.IsInsideExitTest) - { - continue; - } - if ((cursor.IV != nullptr) && ((*nextIV == nullptr) || Scev::Equals(cursor.IV, *nextIV))) { *nextIV = cursor.IV; @@ -1675,50 +1751,6 @@ bool StrengthReductionContext::CheckAdvancedCursors(ArrayStack* curs return false; } - // Now check all exit test uses. - for (int i = 0; i < cursors->Height(); i++) - { - CursorInfo& cursor = cursors->BottomRef(i); - - if (!cursor.IsInsideExitTest) - { - continue; - } - - if ((cursor.IV != nullptr) && ((*nextIV == nullptr) || Scev::Equals(cursor.IV, *nextIV))) - { - *nextIV = cursor.IV; - continue; - } - - // Use inside exit test does not match. - if (derivedLevel <= 1) - { - // We weren't able to advance the match in the exit test at all; in - // this situation we expect the downwards optimization to be able - // to remove the use of the primary IV, so this is ok. Remove the - // cursor pointing to the use inside the test. - JITDUMP(" [%d] does not match, but is inside loop test; ignoring mismatch and removing cursor\n", i); - - std::swap(m_cursors1.BottomRef(i), m_cursors1.TopRef(0)); - std::swap(m_cursors2.BottomRef(i), m_cursors2.TopRef(0)); - - m_cursors1.Pop(); - m_cursors2.Pop(); - - i--; - } - else - { - // We already found a derived IV in the exit test that matches, so - // stop here and allow the replacement to replace the uses of the - // current derived IV, including the one in the exit test - // statement. - JITDUMP(" [%d] does not match; will not advance\n", i); - return false; - } - } - return *nextIV != nullptr; } From 31abd438ece3a018b21c1fe80e55a975a7dba719 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 17 Jul 2024 14:20:13 +0200 Subject: [PATCH 3/5] Fix docs --- src/coreclr/jit/inductionvariableopts.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/inductionvariableopts.cpp b/src/coreclr/jit/inductionvariableopts.cpp index 7b1d4020f12c6b..be5bce3de4b3bb 100644 --- a/src/coreclr/jit/inductionvariableopts.cpp +++ b/src/coreclr/jit/inductionvariableopts.cpp @@ -1158,6 +1158,18 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte return true; } +//------------------------------------------------------------------------ +// optCanAndShouldChangeExitTest: +// Check if the exit test can be rephrased to a downwards counted exit test +// (being compared to zero). +// +// Parameters: +// cond - The exit test +// dump - Whether to JITDUMP the reason for the decisions +// +// Returns: +// True if the exit test can be changed. +// bool Compiler::optCanAndShouldChangeExitTest(GenTree* cond, bool dump) { if ((cond->gtFlags & GTF_SIDE_EFFECT) != 0) @@ -1185,7 +1197,7 @@ bool Compiler::optCanAndShouldChangeExitTest(GenTree* cond, bool dump) } //------------------------------------------------------------------------ -// optPrimaryIVIsLoopScoped: +// optPrimaryIVHasNonLoopUses: // Check if a primary IV may have uses of the primary IV that we do not // reason about. // From 0baa588917cef030944013fd634d5085e1bdef2a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 17 Jul 2024 14:21:31 +0200 Subject: [PATCH 4/5] Remove unnecessary change --- src/coreclr/jit/inductionvariableopts.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/inductionvariableopts.cpp b/src/coreclr/jit/inductionvariableopts.cpp index be5bce3de4b3bb..33dad43784e9e3 100644 --- a/src/coreclr/jit/inductionvariableopts.cpp +++ b/src/coreclr/jit/inductionvariableopts.cpp @@ -1010,12 +1010,12 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte } bool hasUseInTest = false; - auto checkRemovableUse = [=, &hasUseInTest](BasicBlock* block, Statement* stmt, GenTree* tree) { + auto checkRemovableUse = [=, &hasUseInTest](BasicBlock* block, Statement* stmt) { if (stmt == jtrueStmt) { hasUseInTest = true; // Use is inside the loop test that we know we can change (from - // calling optCanChangeExitTest above) + // calling optCanAndShouldChangeExitTest above) return true; } @@ -1044,7 +1044,7 @@ bool Compiler::optMakeExitTestDownwardsCounted(ScalarEvolutionContext& scevConte return true; }; - if (!loopLocals->VisitOccurrences(loop, candidateLclNum, checkRemovableUse)) + if (!loopLocals->VisitStatementsWithOccurrences(loop, candidateLclNum, checkRemovableUse)) { // Aborted means we found a non-removable use continue; From a054b04c8c8624c2aa0d84827b2fedf54ac6e2e7 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 17 Jul 2024 14:22:35 +0200 Subject: [PATCH 5/5] Nit --- src/coreclr/jit/inductionvariableopts.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/jit/inductionvariableopts.cpp b/src/coreclr/jit/inductionvariableopts.cpp index 33dad43784e9e3..209e48cccfcabd 100644 --- a/src/coreclr/jit/inductionvariableopts.cpp +++ b/src/coreclr/jit/inductionvariableopts.cpp @@ -1179,7 +1179,10 @@ bool Compiler::optCanAndShouldChangeExitTest(GenTree* cond, bool dump) // to be never analyzable for us even if we tried to do that, so just // do the easy check here. if (dump) + { JITDUMP(" No; exit node has side effects\n"); + } + return false; } @@ -1189,7 +1192,10 @@ bool Compiler::optCanAndShouldChangeExitTest(GenTree* cond, bool dump) (cond->gtGetOp1()->IsIntegralConst(0) || cond->gtGetOp2()->IsIntegralConst(0))) { if (dump) + { JITDUMP(" No; operand of condition [%06u] is already 0\n", dspTreeID(cond)); + } + return false; }