JIT: less loop inversion for bottom tested loops with no evident IV

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
     //

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

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)
     {

src/coreclr/jit/optimizer.cpp

@@ -1949,30 +1949,125 @@ 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())
         {
             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<CondClassifier>
+        {
+            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())