cleanup

Author: EgorBo

src/coreclr/jit/assertionprop.cpp

@@ -5502,13 +5502,17 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions,
     ValueNum          vnCurIdx      = optConservativeNormalVN(arrBndsChkIdx);
     ValueNum          vnCurLen      = optConservativeNormalVN(arrBndsChkLen);
 
+    // Let's see if we can remove the bounds check based on the ranges.
+    Range idxRng = GetRange(this, arrBndsChkIdx, block, assertions, /*fast*/ true);
+
     auto dropBoundsCheck = [&](INDEBUG(const char* reason)) -> GenTree* {
         JITDUMP("\nRemoving redundant (%s) bounds check in " FMT_BB ":\n", reason, compCurBB->bbNum);
         DISPTREE(tree);
 
         // Extract the side effects of idx and len. We deliberately ignore the potential null-check
         // exception of the length (e.g. GT_ARR_LENGTH): having proven the bounds check redundant, we
         // have also proven the length load is non-faulting. This mirrors the hack in optRemoveRangeCheck.
+        // TODO-Bug: We really should be extracting all side effects from the length and index here.
         GenTree* sideEffList = nullptr;
         gtExtractSideEffList(arrBndsChkLen, &sideEffList, GTF_ASG);
         gtExtractSideEffList(arrBndsChkIdx, &sideEffList);
@@ -5534,63 +5538,26 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions,
         // Do we have a previous range check involving the same 'vnLen' upper bound?
         if (curAssertion.GetOp2().GetVN() == optConservativeNormalVN(arrBndsChkLen))
         {
-            // Do we have the exact same lower bound 'vnIdx'?
-            //       a[i] followed by a[i]
             if (curAssertion.GetOp1().GetVN() == vnCurIdx)
             {
                 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(arrBndsChkIdx->TypeGet()))
-            {
-                return dropBoundsCheck(INDEBUG("a[*] followed by a[0]"));
-            }
-            else
+            else if (idxRng.IsConstantRange() && idxRng.LowerLimit().GetConstant() >= 0)
             {
-                // index1 doesn't have to be a constant, it can be a Phi each predecessor of which is a constant.
-                // The smallest of those is what we can rely on. Example:
-                //
-                //  arr[cond ? 10 : 5] = 0;  // arr is at least 6 elements long
-                //  arr[2] = 0;              // arr must be at least 3 elements long
-                //
-                // or even:
+                // Get the range of the previously checked index for the same array length.
+                Range rngOfPrevIdx = RangeCheck::GetRangeFromAssertions(this, curAssertion.GetOp1().GetVN(), nullptr);
+
+                // We know the range of the previous index, we know the range of the current index.
                 //
-                //  arr[cond ? 10 : 5] = 0; // arr is at least 6 elements long
-                //  arr[cond ? 1 : 2] = 0;  // arr must be at least 3 elements long
+                //  a[prevIdx] = 0; // e.g. prevIdx's range is [5..10]
+                //  a[currIdx] = 0; // e.g. currIdx's range is [0..5] -> drop bounds check for currIdx
                 //
-                auto tryGetMaxOrMinConst = [this](ValueNum vn, bool getMin, int* index) -> bool {
-                    *index = getMin ? INT_MAX : INT_MIN;
-                    return vnStore->VNVisitReachingVNs(vn,
-                                                       [this, index, getMin](ValueNum vn) -> ValueNumStore::VNVisit {
-                        int cns = 0;
-                        if (vnStore->IsVNIntegralConstant(vn, &cns))
-                        {
-                            *index = getMin ? min(*index, cns) : max(*index, cns);
-                            return ValueNumStore::VNVisit::Continue;
-                        }
-                        return ValueNumStore::VNVisit::Abort;
-                    }) == ValueNumStore::VNVisit::Continue;
-                };
-
-                int index1;
-                int index2;
-                if (tryGetMaxOrMinConst(curAssertion.GetOp1().GetVN(), /*min*/ true, &index1) &&
-                    tryGetMaxOrMinConst(vnCurIdx, /*max*/ false, &index2))
+                if (rngOfPrevIdx.IsConstantRange() &&
+                    (rngOfPrevIdx.LowerLimit().GetConstant() >= idxRng.UpperLimit().GetConstant()))
                 {
-                    // It can always be considered as redundant with any previous higher constant value
-                    //       a[K1] followed by a[K2], with K2 >= 0 and K1 >= K2
-                    if (index2 >= 0 && index1 >= index2)
-                    {
-                        return dropBoundsCheck(INDEBUG("a[K1] followed by a[K2], with K2 >= 0 and K1 >= K2"));
-                    }
+                    return dropBoundsCheck(INDEBUG("current index is within the previous range"));
                 }
             }
-            // Extend this to remove additional redundant bounds checks:
-            // i.e.  a[i+1] followed by a[i]  by using the VN(i+1) >= VN(i)
-            //       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
         }
     }
 
@@ -5629,8 +5596,6 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions,
         return dropBoundsCheck(INDEBUG("a[X u% a.Length] is always within bounds"));
     }
 
-    // Let's see if we can remove the bounds check based on the ranges.
-    Range idxRng = GetRange(this, arrBndsChkIdx, block, assertions, /*fast*/ true);
     Range lenRng = GetRange(this, arrBndsChkLen, block, assertions, /*fast*/ true);
 
     if (idxRng.IsConstantRange() && lenRng.IsConstantRange())