Handle more scenarios for loop cloning (#67930)

* Handle following scenarios for loop cloning:

- Increasing loops that are incremented by more than 1 like "i += 2"
- Decreasing loops like "i--", "i -= 2"

* jit format

* Make the loop cloning condition tighter

* check for stride

* Add a check for stride edge scenario

* Add test case

* Review feedback

diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
index b9bfbc7..6569768 100644 (file)
--- a/src/coreclr/jit/compiler.h
+++ b/src/coreclr/jit/compiler.h
@@ -6122,6 +6122,10 @@ public:
         GenTree*   lpTestTree;         // pointer to the node containing the loop test
         genTreeOps lpTestOper() const; // the type of the comparison between the iterator and the limit (GT_LE, GT_GE,
                                        // etc.)
+
+        bool lpIsIncreasingLoop() const; // if the loop iterator increases from low to high value.
+        bool lpIsDecreasingLoop() const; // if the loop iterator decreases from high to low value.
+
         void VERIFY_lpTestTree() const;
 
         bool     lpIsReversed() const; // true if the iterator node is the second operand in the loop condition

diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp
index c1040b1..e77d896 100644 (file)
--- a/src/coreclr/jit/compiler.hpp
+++ b/src/coreclr/jit/compiler.hpp
@@ -3511,6 +3511,27 @@ inline genTreeOps Compiler::LoopDsc::lpTestOper() const
 
 //-----------------------------------------------------------------------------
 
+inline bool Compiler::LoopDsc::lpIsIncreasingLoop() const
+{
+    // Increasing loop is the one that has "+=" increment operation and "< or <=" limit check.
+    bool isLessThanLimitCheck = GenTree::StaticOperIs(lpTestOper(), GT_LT, GT_LE);
+    return (isLessThanLimitCheck &&
+            (((lpIterOper() == GT_ADD) && (lpIterConst() > 0)) || ((lpIterOper() == GT_SUB) && (lpIterConst() < 0))));
+}
+
+//-----------------------------------------------------------------------------
+
+inline bool Compiler::LoopDsc::lpIsDecreasingLoop() const
+{
+    // Decreasing loop is the one that has "-=" decrement operation and "> or >=" limit check. If the operation is
+    // "+=", make sure the constant is negative to give an effect of decrementing the iterator.
+    bool isGreaterThanLimitCheck = GenTree::StaticOperIs(lpTestOper(), GT_GT, GT_GE);
+    return (isGreaterThanLimitCheck &&
+            (((lpIterOper() == GT_ADD) && (lpIterConst() < 0)) || ((lpIterOper() == GT_SUB) && (lpIterConst() > 0))));
+}
+
+//-----------------------------------------------------------------------------
+
 inline GenTree* Compiler::LoopDsc::lpIterator() const
 {
     VERIFY_lpTestTree();

diff --git a/src/coreclr/jit/loopcloning.cpp b/src/coreclr/jit/loopcloning.cpp
index 8f22a48..a5c4d8e 100644 (file)
--- a/src/coreclr/jit/loopcloning.cpp
+++ b/src/coreclr/jit/loopcloning.cpp
@@ -1022,18 +1022,29 @@ bool Compiler::optDeriveLoopCloningConditions(unsigned loopNum, LoopCloneContext
     JITDUMP("------------------------------------------------------------\n");
     JITDUMP("Deriving cloning conditions for " FMT_LP "\n", loopNum);
 
-    LoopDsc*                         loop     = &optLoopTable[loopNum];
-    JitExpandArrayStack<LcOptInfo*>* optInfos = context->GetLoopOptInfo(loopNum);
+    LoopDsc*                         loop             = &optLoopTable[loopNum];
+    JitExpandArrayStack<LcOptInfo*>* optInfos         = context->GetLoopOptInfo(loopNum);
+    bool                             isIncreasingLoop = loop->lpIsIncreasingLoop();
+    assert(isIncreasingLoop || loop->lpIsDecreasingLoop());
 
-    if (GenTree::StaticOperIs(loop->lpTestOper(), GT_LT, GT_LE))
+    if (GenTree::StaticOperIs(loop->lpTestOper(), GT_LT, GT_LE, GT_GT, GT_GE))
     {
-        // Stride conditions
-        if (loop->lpIterConst() <= 0)
+        // We already know that this is either increasing or decreasing loop and the
+        // stride is (> 0) or (< 0). Here, just take the abs() value and check if it
+        // is beyond the limit.
+        int stride = abs(loop->lpIterConst());
+
+        if (stride >= 58)
         {
-            JITDUMP("> Stride %d is invalid\n", loop->lpIterConst());
+            // Array.MaxLength can have maximum of 0X7FFFFFC7 elements, so make sure
+            // the stride increment doesn't overflow or underflow the index. Hence,
+            // the maximum stride limit is set to
+            // (int.MaxValue - (Array.MaxLength - 1) + 1), which is
+            // (0X7fffffff - 0x7fffffc7 + 2) = 0x3a or 58.
             return false;
         }
 
+        LC_Ident ident;
         // Init conditions
         if (loop->lpFlags & LPFLG_CONST_INIT)
         {
@@ -1045,6 +1056,12 @@ bool Compiler::optDeriveLoopCloningConditions(unsigned loopNum, LoopCloneContext
                 JITDUMP("> Init %d is invalid\n", loop->lpConstInit);
                 return false;
             }
+
+            if (!isIncreasingLoop)
+            {
+                // For decreasing loop, the init value needs to be checked against the array length
+                ident = LC_Ident(static_cast<unsigned>(loop->lpConstInit), LC_Ident::Const);
+            }
         }
         else
         {
@@ -1056,13 +1073,22 @@ bool Compiler::optDeriveLoopCloningConditions(unsigned loopNum, LoopCloneContext
                 return false;
             }
 
-            LC_Condition geZero(GT_GE, LC_Expr(LC_Ident(initLcl, LC_Ident::Var)),
-                                LC_Expr(LC_Ident(0, LC_Ident::Const)));
+            LC_Condition geZero;
+            if (isIncreasingLoop)
+            {
+                geZero = LC_Condition(GT_GE, LC_Expr(LC_Ident(initLcl, LC_Ident::Var)),
+                                      LC_Expr(LC_Ident(0, LC_Ident::Const)));
+            }
+            else
+            {
+                // For decreasing loop, the init value needs to be checked against the array length
+                ident  = LC_Ident(initLcl, LC_Ident::Var);
+                geZero = LC_Condition(GT_GE, LC_Expr(ident), LC_Expr(LC_Ident(0, LC_Ident::Const)));
+            }
             context->EnsureConditions(loopNum)->Push(geZero);
         }
 
         // Limit Conditions
-        LC_Ident ident;
         if (loop->lpFlags & LPFLG_CONST_LIMIT)
         {
             int limit = loop->lpConstLimit();
@@ -1071,7 +1097,12 @@ bool Compiler::optDeriveLoopCloningConditions(unsigned loopNum, LoopCloneContext
                 JITDUMP("> limit %d is invalid\n", limit);
                 return false;
             }
-            ident = LC_Ident(static_cast<unsigned>(limit), LC_Ident::Const);
+
+            if (isIncreasingLoop)
+            {
+                // For increasing loop, thelimit value needs to be checked against the array length
+                ident = LC_Ident(static_cast<unsigned>(limit), LC_Ident::Const);
+            }
         }
         else if (loop->lpFlags & LPFLG_VAR_LIMIT)
         {
@@ -1082,8 +1113,18 @@ bool Compiler::optDeriveLoopCloningConditions(unsigned loopNum, LoopCloneContext
                 return false;
             }
 
-            ident = LC_Ident(limitLcl, LC_Ident::Var);
-            LC_Condition geZero(GT_GE, LC_Expr(ident), LC_Expr(LC_Ident(0, LC_Ident::Const)));
+            LC_Condition geZero;
+            if (isIncreasingLoop)
+            {
+                // For increasing loop, thelimit value needs to be checked against the array length
+                ident  = LC_Ident(limitLcl, LC_Ident::Var);
+                geZero = LC_Condition(GT_GE, LC_Expr(ident), LC_Expr(LC_Ident(0, LC_Ident::Const)));
+            }
+            else
+            {
+                geZero = LC_Condition(GT_GE, LC_Expr(LC_Ident(limitLcl, LC_Ident::Var)),
+                                      LC_Expr(LC_Ident(0, LC_Ident::Const)));
+            }
 
             context->EnsureConditions(loopNum)->Push(geZero);
         }
@@ -1107,15 +1148,22 @@ bool Compiler::optDeriveLoopCloningConditions(unsigned loopNum, LoopCloneContext
             return false;
         }
 
-        // GT_LT loop test: limit <= arrLen
-        // GT_LE loop test: limit < arrLen
+        // Increasing loops
+        // GT_LT loop test: (start < end) ==> (end <= arrLen)
+        // GT_LE loop test: (start <= end) ==> (end < arrLen)
+        //
+        // Decreasing loops
+        // GT_GT loop test: (end > start) ==> (end <= arrLen)
+        // GT_GE loop test: (end >= start) ==> (end < arrLen)
         genTreeOps opLimitCondition;
         switch (loop->lpTestOper())
         {
             case GT_LT:
+            case GT_GT:
                 opLimitCondition = GT_LE;
                 break;
             case GT_LE:
+            case GT_GE:
                 opLimitCondition = GT_LT;
                 break;
             default:
@@ -1603,13 +1651,6 @@ bool Compiler::optIsLoopClonable(unsigned loopInd)
         return false;
     }
 
-    // TODO-CQ: CLONE: Mark increasing or decreasing loops.
-    if ((loop.lpIterOper() != GT_ADD) || (loop.lpIterConst() != 1))
-    {
-        JITDUMP("Loop cloning: rejecting loop " FMT_LP ". Loop iteration operator not matching.\n", loopInd);
-        return false;
-    }
-
     if ((loop.lpFlags & LPFLG_CONST_LIMIT) == 0 && (loop.lpFlags & LPFLG_VAR_LIMIT) == 0 &&
         (loop.lpFlags & LPFLG_ARRLEN_LIMIT) == 0)
     {
@@ -1618,8 +1659,12 @@ bool Compiler::optIsLoopClonable(unsigned loopInd)
         return false;
     }
 
-    if (!((GenTree::StaticOperIs(loop.lpTestOper(), GT_LT, GT_LE) && (loop.lpIterOper() == GT_ADD)) ||
-          (GenTree::StaticOperIs(loop.lpTestOper(), GT_GT, GT_GE) && (loop.lpIterOper() == GT_SUB))))
+    // TODO-CQ: Handle other loops like:
+    // - The ones whose limit operator is "==" or "!="
+    // - The incrementing operator is multiple and divide
+    // - The ones that are inverted are not handled here for cases like "i *= 2" because
+    //   they are converted to "i + i".
+    if (!(loop.lpIsIncreasingLoop() || loop.lpIsDecreasingLoop()))
     {
         JITDUMP("Loop cloning: rejecting loop " FMT_LP
                 ". Loop test (%s) doesn't agree with the direction (%s) of the loop.\n",

diff --git a/src/tests/JIT/Directed/Arrays/LoopCloning.cs b/src/tests/JIT/Directed/Arrays/LoopCloning.cs
new file mode 100644 (file)
index 0000000..ccc5e45
--- /dev/null
+++ b/src/tests/JIT/Directed/Arrays/LoopCloning.cs
@@ -0,0 +1,34 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+//
+
+using System;
+using System.Runtime.CompilerServices;
+using System.Runtime.Intrinsics;
+
+public class Program
+{
+    public static unsafe int Main()
+    {
+        int result = 0;
+        try {
+            test_up_big(new int[10], 5, 2);
+        } catch (IndexOutOfRangeException) {
+            result = 100;
+        }
+        
+        return result;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static int test_up_big(int[] a, int s, int x)
+    {
+        int r = 0;
+        int i;
+        for (i = 1; i < s; i += 2147483647)
+        {
+            r += a[i];
+        }
+        return r;
+    }
+}
\ No newline at end of file

diff --git a/src/tests/JIT/Directed/Arrays/LoopCloning.csproj b/src/tests/JIT/Directed/Arrays/LoopCloning.csproj
new file mode 100644 (file)
index 0000000..c8c8924
--- /dev/null
+++ b/src/tests/JIT/Directed/Arrays/LoopCloning.csproj
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>PdbOnly</DebugType>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="LoopCloning.cs" />
+  </ItemGroup>
+</Project>