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 58e3310b66ebf2..12d44f0bb502e6 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,14 +1972,102 @@ 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 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 condCandidateLocals = BitVecOps::UninitVal(); + if (bottomTestedNoIV) + { + assert(analyzedIteration); + + condCandidateLocals = BitVecOps::MakeEmpty(&condTraits); + + struct CondClassifier : GenTreeVisitor + { + BitVecTraits* m_traits; + BitVec* m_candidateLocals; + bool* m_hasCandidate; + + enum + { + DoPreOrder = true + }; + + CondClassifier(Compiler* comp, BitVecTraits* traits, BitVec* candidateLocals, bool* hasCandidate) + : GenTreeVisitor(comp) + , m_traits(traits) + , m_candidateLocals(candidateLocals) + , m_hasCandidate(hasCandidate) + { + } + + void CollectLocals(GenTree* tree) + { + if (tree->OperIsAnyLocal()) + { + BitVecOps::AddElemD(m_traits, *m_candidateLocals, 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()) + { + // 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_hasCandidate = true; + CollectLocals(n->AsIndir()->Addr()); } + return WALK_CONTINUE; } + }; + + CondClassifier cc(this, &condTraits, &condCandidateLocals, &condHasHoistCandidate); + for (Statement* const stmt : condBlock->Statements()) + { + GenTree* root = stmt->GetRootNode(); + cc.WalkTree(&root, nullptr); } } @@ -2051,6 +2146,13 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) { *boundsCheckFlag = true; } + // 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())) + { + condCandidateStored = true; + } loopSize++; return 1; }); @@ -2081,6 +2183,22 @@ bool Compiler::optTryInvertWhileLoop(FlowGraphNaturalLoop* loop) } } + // 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 = condHasHoistCandidate && !condCandidateStored; + if (!keepInverting) + { + JITDUMP("No loop-inversion for " FMT_LP "; already bottom-tested with no recognized IV and no " + "loop-invariant hoisting candidate in the duplicated condition\n", + loop->GetIndex()); + return false; + } + } + unsigned estDupCostSz = 0; for (BasicBlock* const block : duplicatedBlocks.BottomUpOrder())