Cloning improvements (#66257)

* Loop cloning improvements

Fix various comments

* Remove loop cloning var initialization condition

Assume that any pre-existing initialization is ok. Check it against
zero if necessary. Const inits remain as before.

Lots of diffs due to more cloning for cases of `for (i = expression...`
where `expression` is not just a constant or local var.

* Feedback

Author: BruceForstall

src/coreclr/jit/compiler.cpp

@@ -319,7 +319,6 @@ unsigned totalLoopCount;          // counts the total number of natural loops
 unsigned totalUnnatLoopCount;     // counts the total number of (not-necessarily natural) loops
 unsigned totalUnnatLoopOverflows; // # of methods that identified more unnatural loops than we can represent
 unsigned iterLoopCount;           // counts the # of loops with an iterator (for like)
-unsigned simpleTestLoopCount;     // counts the # of loops with an iterator and a simple loop condition (iter < const)
 unsigned constIterLoopCount;      // counts the # of loops with a constant iterator (for like)
 bool     hasMethodLoops;          // flag to keep track if we already counted a method as having loops
 unsigned loopsThisMethod;         // counts the number of loops in the current method
@@ -1556,7 +1555,6 @@ void Compiler::compShutdown()
     fprintf(fout, "Total number of 'unnatural' loops is %5u\n", totalUnnatLoopCount);
     fprintf(fout, "# of methods overflowing unnat loop limit is %5u\n", totalUnnatLoopOverflows);
     fprintf(fout, "Total number of loops with an         iterator is %5u\n", iterLoopCount);
-    fprintf(fout, "Total number of loops with a simple   iterator is %5u\n", simpleTestLoopCount);
     fprintf(fout, "Total number of loops with a constant iterator is %5u\n", constIterLoopCount);
 
     fprintf(fout, "--------------------------------------------------\n");

src/coreclr/jit/compiler.h

@@ -2498,13 +2498,13 @@ enum LoopFlags : unsigned short
 
     // LPFLG_UNUSED  = 0x0001,
     // LPFLG_UNUSED  = 0x0002,
-    LPFLG_ITER = 0x0004, // loop of form: for (i = icon or lclVar; test_condition(); i++)
+    LPFLG_ITER = 0x0004, // loop of form: for (i = icon or expression; test_condition(); i++)
     // LPFLG_UNUSED    = 0x0008,
 
     LPFLG_CONTAINS_CALL = 0x0010, // If executing the loop body *may* execute a call
-    LPFLG_VAR_INIT      = 0x0020, // iterator is initialized with a local var (var # found in lpVarInit)
-    LPFLG_CONST_INIT    = 0x0040, // iterator is initialized with a constant (found in lpConstInit)
-    LPFLG_SIMD_LIMIT    = 0x0080, // iterator is compared with vector element count (found in lpConstLimit)
+    // LPFLG_UNUSED     = 0x0020,
+    LPFLG_CONST_INIT = 0x0040, // iterator is initialized with a constant (found in lpConstInit)
+    LPFLG_SIMD_LIMIT = 0x0080, // iterator is compared with vector element count (found in lpConstLimit)
 
     LPFLG_VAR_LIMIT    = 0x0100, // iterator is compared with a local var (var # found in lpVarLimit)
     LPFLG_CONST_LIMIT  = 0x0200, // iterator is compared with a constant (found in lpConstLimit)
@@ -6929,16 +6929,11 @@ class Compiler
 
         var_types lpIterOperType() const; // For overflow instructions
 
-        // Set to the block where we found the initialization for LPFLG_CONST_INIT or LPFLG_VAR_INIT loops.
+        // Set to the block where we found the initialization for LPFLG_CONST_INIT loops.
         // Initially, this will be 'head', but 'head' might change if we insert a loop pre-header block.
         BasicBlock* lpInitBlock;
 
-        union {
-            int lpConstInit;    // initial constant value of iterator
-                                // : Valid if LPFLG_CONST_INIT
-            unsigned lpVarInit; // initial local var number to which we initialize the iterator
-                                // : Valid if LPFLG_VAR_INIT
-        };
+        int lpConstInit; // initial constant value of iterator : Valid if LPFLG_CONST_INIT
 
         // The following is for LPFLG_ITER loops only (i.e. the loop condition is "i RELOP const or var")
 
@@ -12068,21 +12063,19 @@ extern Histogram bbOneBBSizeTable;
 
 #if COUNT_LOOPS
 
-extern unsigned totalLoopMethods;        // counts the total number of methods that have natural loops
-extern unsigned maxLoopsPerMethod;       // counts the maximum number of loops a method has
-extern unsigned totalLoopOverflows;      // # of methods that identified more loops than we can represent
-extern unsigned totalLoopCount;          // counts the total number of natural loops
-extern unsigned totalUnnatLoopCount;     // counts the total number of (not-necessarily natural) loops
-extern unsigned totalUnnatLoopOverflows; // # of methods that identified more unnatural loops than we can represent
-extern unsigned iterLoopCount;           // counts the # of loops with an iterator (for like)
-extern unsigned simpleTestLoopCount;     // counts the # of loops with an iterator and a simple loop condition (iter <
-                                         // const)
-extern unsigned  constIterLoopCount;     // counts the # of loops with a constant iterator (for like)
-extern bool      hasMethodLoops;         // flag to keep track if we already counted a method as having loops
-extern unsigned  loopsThisMethod;        // counts the number of loops in the current method
-extern bool      loopOverflowThisMethod; // True if we exceeded the max # of loops in the method.
-extern Histogram loopCountTable;         // Histogram of loop counts
-extern Histogram loopExitCountTable;     // Histogram of loop exit counts
+extern unsigned  totalLoopMethods;        // counts the total number of methods that have natural loops
+extern unsigned  maxLoopsPerMethod;       // counts the maximum number of loops a method has
+extern unsigned  totalLoopOverflows;      // # of methods that identified more loops than we can represent
+extern unsigned  totalLoopCount;          // counts the total number of natural loops
+extern unsigned  totalUnnatLoopCount;     // counts the total number of (not-necessarily natural) loops
+extern unsigned  totalUnnatLoopOverflows; // # of methods that identified more unnatural loops than we can represent
+extern unsigned  iterLoopCount;           // counts the # of loops with an iterator (for like)
+extern unsigned  constIterLoopCount;      // counts the # of loops with a constant iterator (for like)
+extern bool      hasMethodLoops;          // flag to keep track if we already counted a method as having loops
+extern unsigned  loopsThisMethod;         // counts the number of loops in the current method
+extern bool      loopOverflowThisMethod;  // True if we exceeded the max # of loops in the method.
+extern Histogram loopCountTable;          // Histogram of loop counts
+extern Histogram loopExitCountTable;      // Histogram of loop exit counts
 
 #endif // COUNT_LOOPS
 

src/coreclr/jit/fgdiagnostic.cpp

@@ -3781,13 +3781,10 @@ void Compiler::fgDebugCheckLoopTable()
         }
 
         // Check the flags.
-        // Note that the various init/limit flags are only used when LPFLG_ITER is set, but they are set first,
+        // Note that the various limit flags are only used when LPFLG_ITER is set, but they are set first,
         // separately, and only if everything works out is LPFLG_ITER set. If LPFLG_ITER is NOT set, the
         // individual flags are not un-set (arguably, they should be).
 
-        // Only one of the `init` flags can be set.
-        assert(genCountBits((unsigned)(loop.lpFlags & (LPFLG_VAR_INIT | LPFLG_CONST_INIT))) <= 1);
-
         // Only one of the `limit` flags can be set. (Note that LPFLG_SIMD_LIMIT is a "sub-flag" that can be
         // set when LPFLG_CONST_LIMIT is set.)
         assert(genCountBits((unsigned)(loop.lpFlags & (LPFLG_VAR_LIMIT | LPFLG_CONST_LIMIT | LPFLG_ARRLEN_LIMIT))) <=
@@ -3799,14 +3796,9 @@ void Compiler::fgDebugCheckLoopTable()
             assert(loop.lpFlags & LPFLG_CONST_LIMIT);
         }
 
-        if (loop.lpFlags & (LPFLG_CONST_INIT | LPFLG_VAR_INIT))
+        if (loop.lpFlags & LPFLG_CONST_INIT)
         {
             assert(loop.lpInitBlock != nullptr);
-
-            if (loop.lpFlags & LPFLG_VAR_INIT)
-            {
-                assert(loop.lpVarInit < lvaCount);
-            }
         }
 
         if (loop.lpFlags & LPFLG_ITER)

src/coreclr/jit/loopcloning.cpp

@@ -1038,17 +1038,18 @@ bool Compiler::optDeriveLoopCloningConditions(unsigned loopNum, LoopCloneContext
         if (loop->lpFlags & LPFLG_CONST_INIT)
         {
             // Only allowing non-negative const init at this time.
-            // REVIEW: why?
+            // This is because the variable initialized with this constant will be used as an array index,
+            // and array indices must be non-negative.
             if (loop->lpConstInit < 0)
             {
                 JITDUMP("> Init %d is invalid\n", loop->lpConstInit);
                 return false;
             }
         }
-        else if (loop->lpFlags & LPFLG_VAR_INIT)
+        else
         {
-            // initVar >= 0
-            const unsigned initLcl = loop->lpVarInit;
+            // iterVar >= 0
+            const unsigned initLcl = loop->lpIterVar();
             if (!genActualTypeIsInt(lvaGetDesc(initLcl)))
             {
                 JITDUMP("> Init var V%02u not compatible with TYP_INT\n", initLcl);
@@ -1059,11 +1060,6 @@ bool Compiler::optDeriveLoopCloningConditions(unsigned loopNum, LoopCloneContext
                                 LC_Expr(LC_Ident(0, LC_Ident::Const)));
             context->EnsureConditions(loopNum)->Push(geZero);
         }
-        else
-        {
-            JITDUMP("> Not variable init\n");
-            return false;
-        }
 
         // Limit Conditions
         LC_Ident ident;
@@ -1096,7 +1092,7 @@ bool Compiler::optDeriveLoopCloningConditions(unsigned loopNum, LoopCloneContext
             ArrIndex* index = new (getAllocator(CMK_LoopClone)) ArrIndex(getAllocator(CMK_LoopClone));
             if (!loop->lpArrLenLimit(this, index))
             {
-                JITDUMP("> ArrLen not matching");
+                JITDUMP("> ArrLen not matching\n");
                 return false;
             }
             ident = LC_Ident(LC_Array(LC_Array::Jagged, index, LC_Array::ArrLen));
@@ -1823,8 +1819,7 @@ void Compiler::optCloneLoop(unsigned loopInd, LoopCloneContext* context)
 
     // We're going to transform this loop:
     //
-    // H --> E    (or, H conditionally branches around the loop and has fall-through to F == T == E)
-    // F
+    // H --> E    (or, H conditionally branches around the loop and has fall-through to T == E)
     // T
     // E
     // B ?-> T
@@ -1833,14 +1828,12 @@ void Compiler::optCloneLoop(unsigned loopInd, LoopCloneContext* context)
     // to this pair of loops:
     //
     // H ?-> H3   (all loop failure conditions branch to new slow path loop head)
-    // H2--> E    (Optional; if E == T == F, let H fall through to F/T/E)
-    // F
+    // H2--> E    (Optional; if T == E, let H fall through to T/E)
     // T
     // E
     // B  ?-> T
     // X2--> X
-    // H3 --> E2  (aka slowHead. Or, H3 falls through to F2 == T2 == E2)
-    // F2
+    // H3 --> E2  (aka slowHead. Or, H3 falls through to T2 == E2)
     // T2
     // E2
     // B2 ?-> T2
@@ -1908,7 +1901,7 @@ void Compiler::optCloneLoop(unsigned loopInd, LoopCloneContext* context)
     BasicBlock* slowHeadPrev = newPred;
 
     // Now we'll make "h2", after "h" to go to "e" -- unless the loop is a do-while,
-    // so that "h" already falls through to "e" (e == t == f).
+    // so that "h" already falls through to "e" (e == t).
     // It might look like this code is unreachable, since "h" must be a BBJ_ALWAYS, but
     // later we will change "h" to a BBJ_COND along with a set of loop conditions.
     // TODO: it still might be unreachable, since cloning currently is restricted to "do-while" loop forms.
@@ -2100,7 +2093,7 @@ void Compiler::optCloneLoop(unsigned loopInd, LoopCloneContext* context)
 
     BasicBlock* condLast = optInsertLoopChoiceConditions(context, loopInd, slowHead, h);
 
-    // Add the fall-through path pred (either to F/T/E for fall-through from conditions to fast path,
+    // Add the fall-through path pred (either to T/E for fall-through from conditions to fast path,
     // or H2 if branch to E of fast path).
     assert(condLast->bbJumpKind == BBJ_COND);
     JITDUMP("Adding " FMT_BB " -> " FMT_BB "\n", condLast->bbNum, condLast->bbNext->bbNum);

src/coreclr/jit/loopcloning.h

@@ -138,45 +138,37 @@ exception occurs.
     1. Loop detection has completed and the loop table is populated.
 
     2. The loops that will be considered are the ones with the LPFLG_ITER flag:
-       "for (i = icon or lclVar; test_condition(); i++)"
+       "for ( ; test_condition(); i++)"
 
     Limitations
 
-    1. For array based optimizations the loop choice condition is checked
-       before the loop body. This implies that the loop initializer statement
-       has not executed at the time of the check. So any loop cloning condition
-       involving the initial value of the loop counter cannot be condition checked
-       as it hasn't been assigned yet at the time of condition checking. Therefore
-       the initial value has to be statically known. This can be fixed with further
-       effort.
-
-    2. Loops containing nested exception handling regions are not cloned. (Cloning them
+    1. Loops containing nested exception handling regions are not cloned. (Cloning them
        would require creating new exception handling regions for the cloned loop, which
        is "hard".) There are a few other EH-related edge conditions that also cause us to
        reject cloning.
 
-    3. If the loop contains RETURN blocks, and cloning those would push us over the maximum
+    2. If the loop contains RETURN blocks, and cloning those would push us over the maximum
        number of allowed RETURN blocks in the function (either due to GC info encoding limitations
        or otherwise), we reject cloning.
 
-    4. Loop increment must be `i += 1`
+    3. Loop increment must be `i += 1`
 
-    5. Loop test must be `i < x` where `x` is a constant, a variable, or `a.Length` for array `a`
+    4. Loop test must be `i < x` or `i <= x` where `x` is a constant, a variable, or `a.Length` for array `a`
 
-    (There is some implementation support for decrementing loops, but it is incomplete.
-    There is some implementation support for `i <= x` conditions, but it is incomplete
-    (Compiler::optDeriveLoopCloningConditions() only handles GT_LT conditions))
+    (There is some implementation support for decrementing loops, but it is incomplete.)
 
-    6. Loop must have been converted to a do-while form.
+    5. Loop must have been converted to a do-while form.
 
-    7. There are a few other loop well-formedness conditions.
+    6. There are a few other loop well-formedness conditions.
 
-    8. Multi-dimensional (non-jagged) loop index checking is only partially implemented.
+    7. Multi-dimensional (non-jagged) loop index checking is only partially implemented.
 
-    9. Constant initializations and constant limits must be non-negative (REVIEW: why? The
-       implementation does use `unsigned` to represent them.)
+    8. Constant initializations and constant limits must be non-negative. This is because the
+       iterator variable will be used as an array index, and array indices must be non-negative.
+       For non-constant (or not found) iterator variable `i` initialization, we add a dynamic check that
+       `i >= 0`. Constant initializations can be checked statically.
 
-    10. The cloned loop (the slow path) is not added to the loop table, meaning certain
+    9. The cloned loop (the slow path) is not added to the loop table, meaning certain
        downstream optimization passes do not see them. See
        https://github.com/dotnet/runtime/issues/43713.