From e2b3e0c38aeef376f1dfd0867ec8d0eed032ea3e Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 8 Jul 2026 10:53:00 -0700 Subject: [PATCH 1/5] JIT: gate inversion of bottom-tested no-IV loops on a hoisting benefit Under JitLoopInversionRequireBenefitForBottomTested (off by default), skip inverting an already bottom-tested loop with no recognized IV unless the duplicated condition holds a call or a loop-invariant, hoistable load. Targets the arm64 regressions in #130045 while keeping the Dictionary Enumerator.MoveNext inversion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/jitconfigvalues.h | 4 + src/coreclr/jit/optimizer.cpp | 130 +++++++++++++++++++++++++++--- 2 files changed, 125 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index bedf9e217c71c5..14ee28a5be4fba 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -584,6 +584,10 @@ OPT_CONFIG_INTEGER(JitDoLoopHoisting, "JitDoLoopHoisting", 1) // Perform loop OPT_CONFIG_INTEGER(JitDoLoopInversion, "JitDoLoopInversion", 1) // Perform loop inversion on "for/while" loops RELEASE_CONFIG_INTEGER(JitLoopInversionSizeLimit, "JitLoopInversionSizeLimit", 100) // limit inversion to loops with no // more than this many tree nodes +// When set, skip inverting a loop that is already bottom-tested (has an exiting BBJ_COND latch) and +// has no recognized induction variable, unless the loop-continuation test that would be duplicated +// contains a call or a loop-invariant (hence hoistable) memory load. +RELEASE_CONFIG_INTEGER(JitLoopInversionRequireBenefitForBottomTested, "JitLoopInversionRequireBenefitForBottomTested", 0) OPT_CONFIG_INTEGER(JitDoRangeAnalysis, "JitDoRangeAnalysis", 1) // Perform range check analysis OPT_CONFIG_INTEGER(JitDoVNBasedDeadStoreRemoval, "JitDoVNBasedDeadStoreRemoval", 1) // Perform VN-based dead store // removal diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 58e3310b66ebf2..37940eeb372e0d 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -1949,15 +1949,22 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) return (ivTestBlock != nullptr) && (candidate == ivTestBlock); }; + bool sawExitingCondLatch = false; + for (FlowEdge* const backEdge : loop->BackEdges()) { BasicBlock* const latch = backEdge->getSourceBlock(); - if (isExitingCondLatch(latch) && isIvTest(latch)) + if (isExitingCondLatch(latch)) { - JITDUMP("No loop-inversion for " FMT_LP "; IV-test latch " FMT_BB " already makes it bottom-tested\n", - loop->GetIndex(), latch->bbNum); - return false; + sawExitingCondLatch = true; + + if (isIvTest(latch)) + { + JITDUMP("No loop-inversion for " FMT_LP "; IV-test latch " FMT_BB " already makes it bottom-tested\n", + loop->GetIndex(), latch->bbNum); + return false; + } } if (latch->KindIs(BBJ_ALWAYS) && latch->isEmpty()) @@ -1965,13 +1972,96 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) for (FlowEdge* const predEdge : latch->PredEdges()) { BasicBlock* const pred = predEdge->getSourceBlock(); - if (loop->ContainsBlock(pred) && isExitingCondLatch(pred) && isIvTest(pred)) + if (loop->ContainsBlock(pred) && isExitingCondLatch(pred)) { - JITDUMP("No loop-inversion for " FMT_LP "; IV-test predecessor " FMT_BB - " of canonical latch " FMT_BB " already makes it bottom-tested\n", - loop->GetIndex(), pred->bbNum, latch->bbNum); - return false; + sawExitingCondLatch = true; + + if (isIvTest(pred)) + { + JITDUMP("No loop-inversion for " FMT_LP "; IV-test predecessor " FMT_BB + " of canonical latch " FMT_BB " already makes it bottom-tested\n", + loop->GetIndex(), pred->bbNum, latch->bbNum); + return false; + } + } + } + } + } + + // If the loop is already bottom-tested (has an exiting BBJ_COND latch that is not the IV test) + // and no induction variable was recognized, only invert when the block that would be duplicated + // (condBlock) contains a call or a loop-invariant memory load worth hoisting out of the test. + // Classify condBlock here and record the locals used as bases of its indirections; the size-check + // walk below marks whether any such base is assigned in the loop (making the load loop-variant). + const bool checkBenefit = sawExitingCondLatch && (ivTestBlock == nullptr) && + (JitConfig.JitLoopInversionRequireBenefitForBottomTested() != 0); + bool condHasCall = false; + bool condHasIndir = false; + bool condBaseStored = false; + BitVecTraits condTraits(lvaCount, this); + BitVec condIndirBaseLocals(BitVecOps::MakeEmpty(&condTraits)); + if (checkBenefit) + { + assert(analyzedIteration); + + struct CondClassifier : GenTreeVisitor + { + BitVecTraits* m_traits; + BitVec* m_baseLocals; + bool* m_hasCall; + bool* m_hasIndir; + + enum + { + DoPreOrder = true + }; + + CondClassifier(Compiler* comp, BitVecTraits* traits, BitVec* baseLocals, bool* hasCall, bool* hasIndir) + : GenTreeVisitor(comp) + , m_traits(traits) + , m_baseLocals(baseLocals) + , m_hasCall(hasCall) + , m_hasIndir(hasIndir) + { + } + + void CollectLocals(GenTree* tree) + { + if (tree->OperIsLocal()) + { + BitVecOps::AddElemD(m_traits, *m_baseLocals, tree->AsLclVarCommon()->GetLclNum()); + } + tree->VisitOperands([&](GenTree* op) -> GenTree::VisitResult { + CollectLocals(op); + return GenTree::VisitResult::Continue; + }); + } + + fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + GenTree* n = *use; + if (n->IsCall()) + { + *m_hasCall = true; + return WALK_ABORT; + } + if (n->OperIsIndir()) + { + *m_hasIndir = true; + CollectLocals(n->AsIndir()->Addr()); } + return WALK_CONTINUE; + } + }; + + CondClassifier cc(this, &condTraits, &condIndirBaseLocals, &condHasCall, &condHasIndir); + for (Statement* const stmt : condBlock->Statements()) + { + GenTree* root = stmt->GetRootNode(); + cc.WalkTree(&root, nullptr); + if (condHasCall) + { + break; } } } @@ -2051,6 +2141,13 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) { *boundsCheckFlag = true; } + // Note whether any local used as a base of the condition's indirections is stored in + // the loop (making that load loop-variant). + if (checkBenefit && !condHasCall && tree->OperIsLocalStore() && + BitVecOps::IsMember(&condTraits, condIndirBaseLocals, tree->AsLclVarCommon()->GetLclNum())) + { + condBaseStored = true; + } loopSize++; return 1; }); @@ -2081,6 +2178,21 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) } } + // Skip the inversion unless the duplicated test carries a benefit: a call, or an indirection off + // a base not assigned in the loop (a loop-invariant, hoistable load). condBaseStored is left + // conservatively false if the size walk above was skipped or aborted early, keeping the inversion. + if (checkBenefit) + { + const bool keepInverting = condHasCall || (condHasIndir && !condBaseStored); + if (!keepInverting) + { + JITDUMP("No loop-inversion for " FMT_LP "; already bottom-tested with no recognized IV and no " + "hoistable/call benefit in the duplicated condition\n", + loop->GetIndex()); + return false; + } + } + unsigned estDupCostSz = 0; for (BasicBlock* const block : duplicatedBlocks.BottomUpOrder()) From b016750d293e41cc71436b41c1787405e5e214c8 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 8 Jul 2026 18:39:37 -0700 Subject: [PATCH 2/5] JIT: address review feedback; clang-format Defer the benefit-gate bitvec allocation to when the config is on, correct the "indirection address locals" wording, and soften the config comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/jitconfigvalues.h | 6 +++-- src/coreclr/jit/optimizer.cpp | 43 +++++++++++++++++-------------- 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 14ee28a5be4fba..097c8cdc5fb34f 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -586,8 +586,10 @@ RELEASE_CONFIG_INTEGER(JitLoopInversionSizeLimit, "JitLoopInversionSizeLimit", 1 // more than this many tree nodes // When set, skip inverting a loop that is already bottom-tested (has an exiting BBJ_COND latch) and // has no recognized induction variable, unless the loop-continuation test that would be duplicated -// contains a call or a loop-invariant (hence hoistable) memory load. -RELEASE_CONFIG_INTEGER(JitLoopInversionRequireBenefitForBottomTested, "JitLoopInversionRequireBenefitForBottomTested", 0) +// contains a call or a memory load whose address has no loop-varying local. +RELEASE_CONFIG_INTEGER(JitLoopInversionRequireBenefitForBottomTested, + "JitLoopInversionRequireBenefitForBottomTested", + 0) OPT_CONFIG_INTEGER(JitDoRangeAnalysis, "JitDoRangeAnalysis", 1) // Perform range check analysis OPT_CONFIG_INTEGER(JitDoVNBasedDeadStoreRemoval, "JitDoVNBasedDeadStoreRemoval", 1) // Perform VN-based dead store // removal diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 37940eeb372e0d..5014671a718086 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -1990,24 +1990,27 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) // If the loop is already bottom-tested (has an exiting BBJ_COND latch that is not the IV test) // and no induction variable was recognized, only invert when the block that would be duplicated - // (condBlock) contains a call or a loop-invariant memory load worth hoisting out of the test. - // Classify condBlock here and record the locals used as bases of its indirections; the size-check - // walk below marks whether any such base is assigned in the loop (making the load loop-variant). - const bool checkBenefit = sawExitingCondLatch && (ivTestBlock == nullptr) && + // (condBlock) contains a call or a memory load whose address has no loop-varying local. Classify + // condBlock here and record the locals appearing in its indirection address expressions; the + // size-check walk below flags whether any of them is assigned in the loop (making the load + // loop-variant). + const bool checkBenefit = sawExitingCondLatch && (ivTestBlock == nullptr) && (JitConfig.JitLoopInversionRequireBenefitForBottomTested() != 0); - bool condHasCall = false; - bool condHasIndir = false; - bool condBaseStored = false; + bool condHasCall = false; + bool condHasIndir = false; + bool condAddrStored = false; BitVecTraits condTraits(lvaCount, this); - BitVec condIndirBaseLocals(BitVecOps::MakeEmpty(&condTraits)); + BitVec condIndirAddrLocals = BitVecOps::UninitVal(); if (checkBenefit) { assert(analyzedIteration); + condIndirAddrLocals = BitVecOps::MakeEmpty(&condTraits); + struct CondClassifier : GenTreeVisitor { BitVecTraits* m_traits; - BitVec* m_baseLocals; + BitVec* m_addrLocals; bool* m_hasCall; bool* m_hasIndir; @@ -2016,10 +2019,10 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) DoPreOrder = true }; - CondClassifier(Compiler* comp, BitVecTraits* traits, BitVec* baseLocals, bool* hasCall, bool* hasIndir) + CondClassifier(Compiler* comp, BitVecTraits* traits, BitVec* addrLocals, bool* hasCall, bool* hasIndir) : GenTreeVisitor(comp) , m_traits(traits) - , m_baseLocals(baseLocals) + , m_addrLocals(addrLocals) , m_hasCall(hasCall) , m_hasIndir(hasIndir) { @@ -2029,7 +2032,7 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) { if (tree->OperIsLocal()) { - BitVecOps::AddElemD(m_traits, *m_baseLocals, tree->AsLclVarCommon()->GetLclNum()); + BitVecOps::AddElemD(m_traits, *m_addrLocals, tree->AsLclVarCommon()->GetLclNum()); } tree->VisitOperands([&](GenTree* op) -> GenTree::VisitResult { CollectLocals(op); @@ -2054,7 +2057,7 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) } }; - CondClassifier cc(this, &condTraits, &condIndirBaseLocals, &condHasCall, &condHasIndir); + CondClassifier cc(this, &condTraits, &condIndirAddrLocals, &condHasCall, &condHasIndir); for (Statement* const stmt : condBlock->Statements()) { GenTree* root = stmt->GetRootNode(); @@ -2141,12 +2144,12 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) { *boundsCheckFlag = true; } - // Note whether any local used as a base of the condition's indirections is stored in - // the loop (making that load loop-variant). + // Note whether any local appearing in the condition's indirection addresses is stored + // in the loop (making that load loop-variant). if (checkBenefit && !condHasCall && tree->OperIsLocalStore() && - BitVecOps::IsMember(&condTraits, condIndirBaseLocals, tree->AsLclVarCommon()->GetLclNum())) + BitVecOps::IsMember(&condTraits, condIndirAddrLocals, tree->AsLclVarCommon()->GetLclNum())) { - condBaseStored = true; + condAddrStored = true; } loopSize++; return 1; @@ -2179,11 +2182,11 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) } // Skip the inversion unless the duplicated test carries a benefit: a call, or an indirection off - // a base not assigned in the loop (a loop-invariant, hoistable load). condBaseStored is left - // conservatively false if the size walk above was skipped or aborted early, keeping the inversion. + // an address with no loop-varying local. condAddrStored is left conservatively false if the size + // walk above was skipped or aborted early, keeping the inversion. if (checkBenefit) { - const bool keepInverting = condHasCall || (condHasIndir && !condBaseStored); + const bool keepInverting = condHasCall || (condHasIndir && !condAddrStored); if (!keepInverting) { JITDUMP("No loop-inversion for " FMT_LP "; already bottom-tested with no recognized IV and no " From 0a47d9dd3a617cd63ae81c66c8c4d16dd1d67e28 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 14 Jul 2026 15:16:23 -0700 Subject: [PATCH 3/5] JIT: enable bottom-tested loop-inversion benefit gate by default Remove the JitLoopInversionRequireBenefitForBottomTested config and always apply the gate: for an already bottom-tested loop with no recognized IV, only invert when the duplicated condition has a call or an indirection whose address has no loop-varying local. Also address review feedback: track GT_LCL_ADDR in indirection addresses via OperIsAnyLocal, and describe the check as an indirection rather than a load. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/jitconfigvalues.h | 6 ------ src/coreclr/jit/optimizer.cpp | 23 +++++++++++------------ 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 163c248cbcf89a..9c5732d0609963 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -585,12 +585,6 @@ OPT_CONFIG_INTEGER(JitDoLoopHoisting, "JitDoLoopHoisting", 1) // Perform loop OPT_CONFIG_INTEGER(JitDoLoopInversion, "JitDoLoopInversion", 1) // Perform loop inversion on "for/while" loops RELEASE_CONFIG_INTEGER(JitLoopInversionSizeLimit, "JitLoopInversionSizeLimit", 100) // limit inversion to loops with no // more than this many tree nodes -// When set, skip inverting a loop that is already bottom-tested (has an exiting BBJ_COND latch) and -// has no recognized induction variable, unless the loop-continuation test that would be duplicated -// contains a call or a memory load whose address has no loop-varying local. -RELEASE_CONFIG_INTEGER(JitLoopInversionRequireBenefitForBottomTested, - "JitLoopInversionRequireBenefitForBottomTested", - 0) OPT_CONFIG_INTEGER(JitDoRangeAnalysis, "JitDoRangeAnalysis", 1) // Perform range check analysis OPT_CONFIG_INTEGER(JitDoVNBasedDeadStoreRemoval, "JitDoVNBasedDeadStoreRemoval", 1) // Perform VN-based dead store // removal diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 5014671a718086..0cadd4f12ebc7e 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -1990,18 +1990,17 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) // If the loop is already bottom-tested (has an exiting BBJ_COND latch that is not the IV test) // and no induction variable was recognized, only invert when the block that would be duplicated - // (condBlock) contains a call or a memory load whose address has no loop-varying local. Classify + // (condBlock) contains a call or an indirection whose address has no loop-varying local. Classify // condBlock here and record the locals appearing in its indirection address expressions; the - // size-check walk below flags whether any of them is assigned in the loop (making the load + // size-check walk below flags whether any of them is assigned in the loop (making the indirection // loop-variant). - const bool checkBenefit = sawExitingCondLatch && (ivTestBlock == nullptr) && - (JitConfig.JitLoopInversionRequireBenefitForBottomTested() != 0); - bool condHasCall = false; - bool condHasIndir = false; - bool condAddrStored = false; + const bool bottomTestedNoIV = sawExitingCondLatch && (ivTestBlock == nullptr); + bool condHasCall = false; + bool condHasIndir = false; + bool condAddrStored = false; BitVecTraits condTraits(lvaCount, this); BitVec condIndirAddrLocals = BitVecOps::UninitVal(); - if (checkBenefit) + if (bottomTestedNoIV) { assert(analyzedIteration); @@ -2030,7 +2029,7 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) void CollectLocals(GenTree* tree) { - if (tree->OperIsLocal()) + if (tree->OperIsAnyLocal()) { BitVecOps::AddElemD(m_traits, *m_addrLocals, tree->AsLclVarCommon()->GetLclNum()); } @@ -2145,8 +2144,8 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) *boundsCheckFlag = true; } // Note whether any local appearing in the condition's indirection addresses is stored - // in the loop (making that load loop-variant). - if (checkBenefit && !condHasCall && tree->OperIsLocalStore() && + // in the loop (making that indirection loop-variant). + if (bottomTestedNoIV && !condHasCall && tree->OperIsLocalStore() && BitVecOps::IsMember(&condTraits, condIndirAddrLocals, tree->AsLclVarCommon()->GetLclNum())) { condAddrStored = true; @@ -2184,7 +2183,7 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) // Skip the inversion unless the duplicated test carries a benefit: a call, or an indirection off // an address with no loop-varying local. condAddrStored is left conservatively false if the size // walk above was skipped or aborted early, keeping the inversion. - if (checkBenefit) + if (bottomTestedNoIV) { const bool keepInverting = condHasCall || (condHasIndir && !condAddrStored); if (!keepInverting) From 9b952f163839f9010ae9f3c0801c5a1308cd4965 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 14 Jul 2026 15:23:37 -0700 Subject: [PATCH 4/5] JIT: reword loop-inversion JITDUMP to match the implemented heuristic The message claimed a "hoistable" benefit, but the check only requires a call or an indirection whose address has no loop-varying local (which does not by itself prove the load is hoistable). Describe it accurately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/optimizer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 0cadd4f12ebc7e..3cb05c0c9b7cd9 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2189,7 +2189,7 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) if (!keepInverting) { JITDUMP("No loop-inversion for " FMT_LP "; already bottom-tested with no recognized IV and no " - "hoistable/call benefit in the duplicated condition\n", + "call or non-loop-varying indirection in the duplicated condition\n", loop->GetIndex()); return false; } From 14fabd8de5f4d59307a8eb606fc9edc40c8bbd13 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 16 Jul 2026 14:32:51 -0700 Subject: [PATCH 5/5] JIT: use CSE code to vet calls Screen a bottom-tested/no-IV loop condition's calls with optIsCSEcandidate (no persistent side effects, not an allocator) instead of accepting any call, and require the call's arguments to be loop-invariant, matching the treatment of indirections. optIsCSEcandidate/CanConsiderTree gain a skipCostChecks arg so the structural filter can run during loop inversion before tree costs are set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/optcse.cpp | 46 ++++++++++++++------- src/coreclr/jit/optcse.h | 2 +- src/coreclr/jit/optimizer.cpp | 76 ++++++++++++++++++----------------- 4 files changed, 73 insertions(+), 53 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index b2acfa5e21f7db..dc333fdb85181c 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7894,7 +7894,7 @@ class Compiler weight_t optCSEweight; // The weight of the current block when we are doing PerformCSE CSE_HeuristicCommon* optCSEheuristic = nullptr; // CSE Heuristic to use for this method - bool optIsCSEcandidate(GenTree* tree, bool isReturn = false); + bool optIsCSEcandidate(GenTree* tree, bool isReturn = false, bool skipCostChecks = false); // lclNumIsTrueCSE returns true if the LclVar was introduced by the CSE phase of the compiler // diff --git a/src/coreclr/jit/optcse.cpp b/src/coreclr/jit/optcse.cpp index d14d9ebd570bb9..3d9e8c2b0fef0c 100644 --- a/src/coreclr/jit/optcse.cpp +++ b/src/coreclr/jit/optcse.cpp @@ -1773,6 +1773,9 @@ CSE_HeuristicCommon::CSE_HeuristicCommon(Compiler* pCompiler) // Arguments: // tree - tree in question // isReturn - true if tree is part of a return statement +// skipCostChecks - true to skip the cost-based profitability checks (which require initialized +// tree costs); used by callers that only need the legality/structural filter and +// may run before costs are set (e.g. loop inversion) // // Returns: // true if this tree can be a CSE candidate @@ -1781,7 +1784,7 @@ CSE_HeuristicCommon::CSE_HeuristicCommon(Compiler* pCompiler) // This currently does both legality and profitability checks. // Eventually it should just do legality checks. // -bool CSE_HeuristicCommon::CanConsiderTree(GenTree* tree, bool isReturn) +bool CSE_HeuristicCommon::CanConsiderTree(GenTree* tree, bool isReturn, bool skipCostChecks) { // Don't allow CSE of constants if it is disabled // @@ -1833,21 +1836,24 @@ bool CSE_HeuristicCommon::CanConsiderTree(GenTree* tree, bool isReturn) return false; } - unsigned cost; - if (codeOptKind == Compiler::SMALL_CODE) - { - cost = tree->GetCostSz(); - } - else - { - cost = tree->GetCostEx(); - } - // Don't bother if the potential savings are very low // - if (cost < Compiler::MIN_CSE_COST) + if (!skipCostChecks) { - return false; + unsigned cost; + if (codeOptKind == Compiler::SMALL_CODE) + { + cost = tree->GetCostSz(); + } + else + { + cost = tree->GetCostEx(); + } + + if (cost < Compiler::MIN_CSE_COST) + { + return false; + } } genTreeOps oper = tree->OperGet(); @@ -5722,9 +5728,19 @@ PhaseStatus Compiler::optOptimizeValnumCSEs() // // Consults the CSE policy that will be used. // -bool Compiler::optIsCSEcandidate(GenTree* tree, bool isReturn) +bool Compiler::optIsCSEcandidate(GenTree* tree, bool isReturn, bool skipCostChecks) { - return optGetCSEheuristic()->ConsiderTree(tree, isReturn); + CSE_HeuristicCommon* const heuristic = optGetCSEheuristic(); + + // When skipping cost checks the caller only wants the legality/structural filter (e.g. it runs + // before tree costs are initialized), so use CanConsiderTree directly rather than the heuristic's + // cost-aware ConsiderTree. + if (skipCostChecks) + { + return heuristic->CanConsiderTree(tree, isReturn, /* skipCostChecks */ true); + } + + return heuristic->ConsiderTree(tree, isReturn); } #ifdef DEBUG diff --git a/src/coreclr/jit/optcse.h b/src/coreclr/jit/optcse.h index 4dd088afb78c0e..d6056e96701e7c 100644 --- a/src/coreclr/jit/optcse.h +++ b/src/coreclr/jit/optcse.h @@ -77,7 +77,7 @@ class CSE_HeuristicCommon // eventually it should just be pure legality and // the derived classes handle the profitability. // - bool CanConsiderTree(GenTree* tree, bool isReturn); + bool CanConsiderTree(GenTree* tree, bool isReturn, bool skipCostChecks = false); virtual bool ConsiderTree(GenTree* tree, bool isReturn) { diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 3cb05c0c9b7cd9..12d44f0bb502e6 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -1990,40 +1990,39 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) // If the loop is already bottom-tested (has an exiting BBJ_COND latch that is not the IV test) // and no induction variable was recognized, only invert when the block that would be duplicated - // (condBlock) contains a call or an indirection whose address has no loop-varying local. Classify - // condBlock here and record the locals appearing in its indirection address expressions; the - // size-check walk below flags whether any of them is assigned in the loop (making the indirection - // loop-variant). - const bool bottomTestedNoIV = sawExitingCondLatch && (ivTestBlock == nullptr); - bool condHasCall = false; - bool condHasIndir = false; - bool condAddrStored = false; + // (condBlock) contains a loop-invariant hoisting candidate: a CSE-able call, or an indirection, + // whose operands (call arguments / indirection address) have no loop-varying local. Once the body + // dominates the back-edge such a computation can be hoisted by LICM, which is the benefit that + // makes bottom-testing worthwhile. Classify condBlock here and record the candidate operand + // locals; the size-check walk below flags whether any of them is assigned in the loop (making the + // candidate loop-variant). + const bool bottomTestedNoIV = sawExitingCondLatch && (ivTestBlock == nullptr); + bool condHasHoistCandidate = false; + bool condCandidateStored = false; BitVecTraits condTraits(lvaCount, this); - BitVec condIndirAddrLocals = BitVecOps::UninitVal(); + BitVec condCandidateLocals = BitVecOps::UninitVal(); if (bottomTestedNoIV) { assert(analyzedIteration); - condIndirAddrLocals = BitVecOps::MakeEmpty(&condTraits); + condCandidateLocals = BitVecOps::MakeEmpty(&condTraits); struct CondClassifier : GenTreeVisitor { BitVecTraits* m_traits; - BitVec* m_addrLocals; - bool* m_hasCall; - bool* m_hasIndir; + BitVec* m_candidateLocals; + bool* m_hasCandidate; enum { DoPreOrder = true }; - CondClassifier(Compiler* comp, BitVecTraits* traits, BitVec* addrLocals, bool* hasCall, bool* hasIndir) + CondClassifier(Compiler* comp, BitVecTraits* traits, BitVec* candidateLocals, bool* hasCandidate) : GenTreeVisitor(comp) , m_traits(traits) - , m_addrLocals(addrLocals) - , m_hasCall(hasCall) - , m_hasIndir(hasIndir) + , m_candidateLocals(candidateLocals) + , m_hasCandidate(hasCandidate) { } @@ -2031,7 +2030,7 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) { if (tree->OperIsAnyLocal()) { - BitVecOps::AddElemD(m_traits, *m_addrLocals, tree->AsLclVarCommon()->GetLclNum()); + BitVecOps::AddElemD(m_traits, *m_candidateLocals, tree->AsLclVarCommon()->GetLclNum()); } tree->VisitOperands([&](GenTree* op) -> GenTree::VisitResult { CollectLocals(op); @@ -2044,27 +2043,31 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) GenTree* n = *use; if (n->IsCall()) { - *m_hasCall = true; - return WALK_ABORT; + // Only a call the CSE heuristic would consider (no persistent side effects, not an + // allocator) is a reuse candidate; LICM/CSE cannot lift a call with side effects, + // so inverting for it buys nothing. Its arguments must also be loop-invariant. Skip + // the cost-based checks: tree costs are not initialized this early. + if (m_compiler->optIsCSEcandidate(n, /* isReturn */ false, /* skipCostChecks */ true)) + { + *m_hasCandidate = true; + CollectLocals(n); + } + return WALK_CONTINUE; } if (n->OperIsIndir()) { - *m_hasIndir = true; + *m_hasCandidate = true; CollectLocals(n->AsIndir()->Addr()); } return WALK_CONTINUE; } }; - CondClassifier cc(this, &condTraits, &condIndirAddrLocals, &condHasCall, &condHasIndir); + CondClassifier cc(this, &condTraits, &condCandidateLocals, &condHasHoistCandidate); for (Statement* const stmt : condBlock->Statements()) { GenTree* root = stmt->GetRootNode(); cc.WalkTree(&root, nullptr); - if (condHasCall) - { - break; - } } } @@ -2143,12 +2146,12 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) { *boundsCheckFlag = true; } - // Note whether any local appearing in the condition's indirection addresses is stored - // in the loop (making that indirection loop-variant). - if (bottomTestedNoIV && !condHasCall && tree->OperIsLocalStore() && - BitVecOps::IsMember(&condTraits, condIndirAddrLocals, tree->AsLclVarCommon()->GetLclNum())) + // Note whether any local appearing in a condition hoisting candidate's operands is + // stored in the loop (making that candidate loop-variant, hence not hoistable). + if (bottomTestedNoIV && !condCandidateStored && tree->OperIsLocalStore() && + BitVecOps::IsMember(&condTraits, condCandidateLocals, tree->AsLclVarCommon()->GetLclNum())) { - condAddrStored = true; + condCandidateStored = true; } loopSize++; return 1; @@ -2180,16 +2183,17 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) } } - // Skip the inversion unless the duplicated test carries a benefit: a call, or an indirection off - // an address with no loop-varying local. condAddrStored is left conservatively false if the size - // walk above was skipped or aborted early, keeping the inversion. + // Skip the inversion unless the duplicated test carries a benefit: a loop-invariant hoisting + // candidate (a CSE-able call or an indirection whose operands have no loop-varying local), which + // LICM can lift once the body dominates the back-edge. condCandidateStored is left conservatively + // false if the size walk above was skipped or aborted early, keeping the inversion. if (bottomTestedNoIV) { - const bool keepInverting = condHasCall || (condHasIndir && !condAddrStored); + const bool keepInverting = condHasHoistCandidate && !condCandidateStored; if (!keepInverting) { JITDUMP("No loop-inversion for " FMT_LP "; already bottom-tested with no recognized IV and no " - "call or non-loop-varying indirection in the duplicated condition\n", + "loop-invariant hoisting candidate in the duplicated condition\n", loop->GetIndex()); return false; }