Remove redundant bounds checks around arr[arr.Length - cns] pattern (#116105)

Author: EgorBo

src/coreclr/jit/assertionprop.cpp

@@ -5311,6 +5311,50 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree
     }
 #endif // FEATURE_ENABLE_NO_RANGE_CHECKS
 
+    GenTreeBoundsChk* arrBndsChk = tree->AsBoundsChk();
+    ValueNum          vnCurIdx   = vnStore->VNConservativeNormalValue(arrBndsChk->GetIndex()->gtVNPair);
+    ValueNum          vnCurLen   = vnStore->VNConservativeNormalValue(arrBndsChk->GetArrayLength()->gtVNPair);
+
+    auto dropBoundsCheck = [&](INDEBUG(const char* reason)) -> GenTree* {
+        JITDUMP("\nVN based redundant (%s) bounds check assertion prop in " FMT_BB ":\n", reason, compCurBB->bbNum);
+        DISPTREE(tree);
+        if (arrBndsChk != stmt->GetRootNode())
+        {
+            // Defer the removal.
+            arrBndsChk->gtFlags |= GTF_CHK_INDEX_INBND;
+            return nullptr;
+        }
+
+        GenTree* newTree = optRemoveStandaloneRangeCheck(arrBndsChk, stmt);
+        return optAssertionProp_Update(newTree, arrBndsChk, stmt);
+    };
+
+    // First, check if we have arr[arr.Length - cns] when we know arr.Length is >= cns.
+    VNFuncApp funcApp;
+    if (vnStore->GetVNFunc(vnCurIdx, &funcApp) && (funcApp.m_func == VNF_ADD))
+    {
+        if (!vnStore->IsVNInt32Constant(funcApp.m_args[1]))
+        {
+            // Normalize constants to be on the right side
+            std::swap(funcApp.m_args[0], funcApp.m_args[1]);
+        }
+
+        Range rng = Range(Limit(Limit::keUnknown));
+        if ((funcApp.m_args[0] == vnCurLen) && vnStore->IsVNInt32Constant(funcApp.m_args[1]) &&
+            RangeCheck::TryGetRangeFromAssertions(this, vnCurLen, assertions, &rng) && rng.LowerLimit().IsConstant())
+        {
+            // Lower known limit of ArrLen:
+            const int lenLowerLimit = rng.LowerLimit().GetConstant();
+
+            // Negative delta in the array access (ArrLen + -CNS)
+            const int delta = vnStore->GetConstantInt32(funcApp.m_args[1]);
+            if ((lenLowerLimit > 0) && (delta < 0) && (delta > INT_MIN) && (lenLowerLimit >= -delta))
+            {
+                return dropBoundsCheck(INDEBUG("a[a.Length-cns] when a.Length is known to be >= cns"));
+            }
+        }
+    }
+
     BitVecOps::Iter iter(apTraits, assertions);
     unsigned        index = 0;
     while (iter.NextElem(&index))
@@ -5327,38 +5371,21 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree
             continue;
         }
 
-        GenTreeBoundsChk* arrBndsChk = tree->AsBoundsChk();
-
-        // Set 'isRedundant' to true if we can determine that 'arrBndsChk' can be
-        // classified as a redundant bounds check using 'curAssertion'
-        bool isRedundant = false;
-#ifdef DEBUG
-        const char* dbgMsg = "Not Set";
-#endif
-
         // Do we have a previous range check involving the same 'vnLen' upper bound?
         if (curAssertion->op1.bnd.vnLen == vnStore->VNConservativeNormalValue(arrBndsChk->GetArrayLength()->gtVNPair))
         {
-            ValueNum vnCurIdx = vnStore->VNConservativeNormalValue(arrBndsChk->GetIndex()->gtVNPair);
-
             // Do we have the exact same lower bound 'vnIdx'?
             //       a[i] followed by a[i]
             if (curAssertion->op1.bnd.vnIdx == vnCurIdx)
             {
-                isRedundant = true;
-#ifdef DEBUG
-                dbgMsg = "a[i] followed by a[i]";
-#endif
+                return dropBoundsCheck(INDEBUG("a[i] followed by a[i]"));
             }
             // Are we using zero as the index?
             // It can always be considered as redundant with any previous value
             //       a[*] followed by a[0]
             else if (vnCurIdx == vnStore->VNZeroForType(arrBndsChk->GetIndex()->TypeGet()))
             {
-                isRedundant = true;
-#ifdef DEBUG
-                dbgMsg = "a[*] followed by a[0]";
-#endif
+                return dropBoundsCheck(INDEBUG("a[*] followed by a[0]"));
             }
             // Do we have two constant indexes?
             else if (vnStore->IsVNConstant(curAssertion->op1.bnd.vnIdx) && vnStore->IsVNConstant(vnCurIdx))
@@ -5379,10 +5406,7 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree
                     //       a[K1] followed by a[K2], with K2 >= 0 and K1 >= K2
                     if (index2 >= 0 && index1 >= index2)
                     {
-                        isRedundant = true;
-#ifdef DEBUG
-                        dbgMsg = "a[K1] followed by a[K2], with K2 >= 0 and K1 >= K2";
-#endif
+                        return dropBoundsCheck(INDEBUG("a[K1] followed by a[K2], with K2 >= 0 and K1 >= K2"));
                     }
                 }
             }
@@ -5391,35 +5415,6 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree
             //       a[i]   followed by a[j]  when j is known to be >= i
             //       a[i]   followed by a[5]  when i is known to be >= 5
         }
-
-        if (!isRedundant)
-        {
-            continue;
-        }
-
-#ifdef DEBUG
-        if (verbose)
-        {
-            printf("\nVN based redundant (%s) bounds check assertion prop for index #%02u in " FMT_BB ":\n", dbgMsg,
-                   assertionIndex, compCurBB->bbNum);
-            gtDispTree(tree, nullptr, nullptr, true);
-        }
-#endif
-        if (arrBndsChk == stmt->GetRootNode())
-        {
-            // We have a top-level bounds check node.
-            // This can happen when trees are broken up due to inlining.
-            // optRemoveStandaloneRangeCheck will return the modified tree (side effects or a no-op).
-            GenTree* newTree = optRemoveStandaloneRangeCheck(arrBndsChk, stmt);
-
-            return optAssertionProp_Update(newTree, arrBndsChk, stmt);
-        }
-
-        // Defer actually removing the tree until processing reaches its parent comma, since
-        // optRemoveCommaBasedRangeCheck needs to rewrite the whole comma tree.
-        arrBndsChk->gtFlags |= GTF_CHK_INDEX_INBND;
-
-        return nullptr;
     }
 
     return nullptr;

src/coreclr/jit/rangecheck.cpp

@@ -805,11 +805,11 @@ void RangeCheck::MergeEdgeAssertions(Compiler*        comp,
             }
             else if ((normalLclVN == lenVN) && comp->vnStore->IsVNInt32Constant(indexVN))
             {
-                // We have "Const < arr.Length" assertion, it means that "arr.Length >= Const"
+                // We have "Const < arr.Length" assertion, it means that "arr.Length > Const"
                 int indexCns = comp->vnStore->GetConstantInt32(indexVN);
                 if (indexCns >= 0)
                 {
-                    cmpOper = GT_GE;
+                    cmpOper = GT_GT;
                     limit   = Limit(Limit::keConstant, indexCns);
                 }
                 else