Jit: Remove bounds checks with tests against length. (#40180)

* Introduce a concept of minimum array length into range check

* Some cleanup

* fix potential underflow

* bug fix and cleanup

* Revert string changes

* Allow elimination of arr[0] with  len != 0 test

* Revert "Revert string changes"

This reverts commit 6f77bf8.

* Fix up tests

* reverting lower bound merging as it may be unsound

* Fix CI exposed bug and add a couple of test cases

* code review feedback

* comment nit

* feedback

* Add missing break

Author: nathan-moore

src/coreclr/src/jit/assertionprop.cpp

@@ -1625,7 +1625,6 @@ void Compiler::optDebugCheckAssertion(AssertionDsc* assertion)
                     assert(assertion->op2.u1.iconFlags != 0);
                     break;
                 case O1K_LCLVAR:
-                case O1K_ARR_BND:
                     assert((lvaTable[assertion->op1.lcl.lclNum].lvType != TYP_REF) || (assertion->op2.u1.iconVal == 0));
                     break;
                 case O1K_VALUE_NUMBER:
@@ -1959,12 +1958,58 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree)
     {
         std::swap(op1, op2);
     }
+
+    ValueNum op1VN = vnStore->VNConservativeNormalValue(op1->gtVNPair);
+    ValueNum op2VN = vnStore->VNConservativeNormalValue(op2->gtVNPair);
     // If op1 is lcl and op2 is const or lcl, create assertion.
     if ((op1->gtOper == GT_LCL_VAR) &&
         ((op2->OperKind() & GTK_CONST) || (op2->gtOper == GT_LCL_VAR))) // Fix for Dev10 851483
     {
         return optCreateJtrueAssertions(op1, op2, assertionKind);
     }
+    else if (vnStore->IsVNCheckedBound(op1VN) && vnStore->IsVNInt32Constant(op2VN))
+    {
+        assert(relop->OperIs(GT_EQ, GT_NE));
+
+        int con = vnStore->ConstantValue<int>(op2VN);
+        if (con >= 0)
+        {
+            AssertionDsc dsc;
+
+            // For arr.Length != 0, we know that 0 is a valid index
+            // For arr.Length == con, we know that con - 1 is the greatest valid index
+            if (con == 0)
+            {
+                dsc.assertionKind = OAK_NOT_EQUAL;
+                dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(0);
+            }
+            else
+            {
+                dsc.assertionKind = OAK_EQUAL;
+                dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(con - 1);
+            }
+
+            dsc.op1.vn           = op1VN;
+            dsc.op1.kind         = O1K_ARR_BND;
+            dsc.op1.bnd.vnLen    = op1VN;
+            dsc.op2.vn           = vnStore->VNConservativeNormalValue(op2->gtVNPair);
+            dsc.op2.kind         = O2K_CONST_INT;
+            dsc.op2.u1.iconFlags = 0;
+            dsc.op2.u1.iconVal   = 0;
+
+            // when con is not zero, create an assertion on the arr.Length == con edge
+            // when con is zero, create an assertion on the arr.Length != 0 edge
+            AssertionIndex index = optAddAssertion(&dsc);
+            if (relop->OperIs(GT_NE) != (con == 0))
+            {
+                return AssertionInfo::ForNextEdge(index);
+            }
+            else
+            {
+                return index;
+            }
+        }
+    }
 
     // Check op1 and op2 for an indirection of a GT_LCL_VAR and keep it in op1.
     if (((op1->gtOper != GT_IND) || (op1->AsOp()->gtOp1->gtOper != GT_LCL_VAR)) &&

src/coreclr/src/jit/compiler.h

@@ -6912,6 +6912,11 @@ class Compiler
             return ((assertionKind == OAK_EQUAL) && (op1.kind == O1K_LCLVAR) && (op2.kind == O2K_LCLVAR_COPY));
         }
 
+        bool IsConstantInt32Assertion()
+        {
+            return ((assertionKind == OAK_EQUAL) || (assertionKind == OAK_NOT_EQUAL)) && (op2.kind == O2K_CONST_INT);
+        }
+
         static bool SameKind(AssertionDsc* a1, AssertionDsc* a2)
         {
             return a1->assertionKind == a2->assertionKind && a1->op1.kind == a2->op1.kind &&

src/coreclr/src/jit/rangecheck.cpp

@@ -59,13 +59,29 @@ int RangeCheck::GetArrLength(ValueNum vn)
     return m_pCompiler->vnStore->GetNewArrSize(arrRefVN);
 }
 
-// Check if the computed range is within bounds.
-bool RangeCheck::BetweenBounds(Range& range, int lower, GenTree* upper)
+//------------------------------------------------------------------------
+// BetweenBounds: Check if the computed range is within bounds
+//
+// Arguments:
+//    Range - the range to check if in bounds
+//    upper - the array length vn
+//    arrSize - the length of the array if known, or <= 0
+//
+// Return Value:
+//    True iff range is between [0 and vn - 1] or [0, arrSize - 1]
+//
+// notes:
+//    This function assumes that the lower range is resolved and upper range is symbolic as in an
+//    increasing loop.
+//
+// TODO-CQ: This is not general enough.
+//
+bool RangeCheck::BetweenBounds(Range& range, GenTree* upper, int arrSize)
 {
 #ifdef DEBUG
     if (m_pCompiler->verbose)
     {
-        printf("%s BetweenBounds <%d, ", range.ToString(m_pCompiler->getAllocatorDebugOnly()), lower);
+        printf("%s BetweenBounds <%d, ", range.ToString(m_pCompiler->getAllocatorDebugOnly()), 0);
         Compiler::printTreeID(upper);
         printf(">\n");
     }
@@ -85,28 +101,9 @@ bool RangeCheck::BetweenBounds(Range& range, int lower, GenTree* upper)
     JITDUMP("\n");
 #endif
 
-    int arrSize = 0;
-
-    if (vnStore->IsVNConstant(uLimitVN))
+    if ((arrSize <= 0) && !vnStore->IsVNCheckedBound(uLimitVN))
     {
-        ssize_t  constVal  = -1;
-        unsigned iconFlags = 0;
-
-        if (m_pCompiler->optIsTreeKnownIntValue(true, upper, &constVal, &iconFlags))
-        {
-            arrSize = (int)constVal;
-        }
-    }
-    else if (vnStore->IsVNArrLen(uLimitVN))
-    {
-        // Get the array reference from the length.
-        ValueNum arrRefVN = vnStore->GetArrForLenVn(uLimitVN);
-        // Check if array size can be obtained.
-        arrSize = vnStore->GetNewArrSize(arrRefVN);
-    }
-    else if (!vnStore->IsVNCheckedBound(uLimitVN))
-    {
-        // If the upper limit is not length, then bail.
+        // If we don't know the array size and the upper limit is not known, then bail.
         return false;
     }
 
@@ -231,6 +228,19 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
 #endif // FEATURE_SIMD
     {
         arrSize = GetArrLength(arrLenVn);
+
+        // if we can't find the array length, see if there
+        // are any assertions about the array size we can use to get a minimum length
+        if (arrSize <= 0)
+        {
+            JITDUMP("Looking for array size assertions for: " FMT_VN "\n", arrLenVn);
+            Range arrLength = Range(Limit(Limit::keDependent));
+            MergeEdgeAssertions(arrLenVn, block->bbAssertionIn, &arrLength);
+            if (arrLength.lLimit.IsConstant())
+            {
+                arrSize = arrLength.lLimit.GetConstant();
+            }
+        }
     }
 
     JITDUMP("ArrSize for lengthVN:%03X = %d\n", arrLenVn, arrSize);
@@ -286,7 +296,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
     }
 
     // Is the range between the lower and upper bound values.
-    if (BetweenBounds(range, 0, bndsChk->gtArrLen))
+    if (BetweenBounds(range, bndsChk->gtArrLen, arrSize))
     {
         JITDUMP("[RangeCheck::OptimizeRangeCheck] Between bounds\n");
         m_pCompiler->optRemoveRangeCheck(treeParent, stmt);
@@ -532,18 +542,51 @@ void RangeCheck::SetDef(UINT64 hash, Location* loc)
 }
 #endif
 
-// Merge assertions on the edge flowing into the block about a variable.
+//------------------------------------------------------------------------
+// MergeEdgeAssertions: Merge assertions on the edge flowing into the block about a variable
+//
+// Arguments:
+//    GenTreeLclVarCommon - the variable to look for assertions for
+//    assertions - the assertions to use
+//    pRange - the range to tighten with assertions
+//
 void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP assertions, Range* pRange)
+{
+    if (lcl->GetSsaNum() == SsaConfig::RESERVED_SSA_NUM)
+    {
+        return;
+    }
+
+    LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lcl);
+    if (varDsc->CanBeReplacedWithItsField(m_pCompiler))
+    {
+        varDsc = m_pCompiler->lvaGetDesc(varDsc->lvFieldLclStart);
+    }
+    LclSsaVarDsc* ssaData     = varDsc->GetPerSsaData(lcl->GetSsaNum());
+    ValueNum      normalLclVN = m_pCompiler->vnStore->VNConservativeNormalValue(ssaData->m_vnPair);
+    MergeEdgeAssertions(normalLclVN, assertions, pRange);
+}
+
+//------------------------------------------------------------------------
+// MergeEdgeAssertions: Merge assertions on the edge flowing into the block about a variable
+//
+// Arguments:
+//    normalLclVN - the value number to look for assertions for
+//    assertions - the assertions to use
+//    pRange - the range to tighten with assertions
+//
+void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP assertions, Range* pRange)
 {
     if (BitVecOps::IsEmpty(m_pCompiler->apTraits, assertions))
     {
         return;
     }
 
-    if (lcl->GetSsaNum() == SsaConfig::RESERVED_SSA_NUM)
+    if (normalLclVN == ValueNumStore::NoVN)
     {
         return;
     }
+
     // Walk through the "assertions" to check if the apply.
     BitVecOps::Iter iter(m_pCompiler->apTraits, assertions);
     unsigned        index = 0;
@@ -554,15 +597,8 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
         Compiler::AssertionDsc* curAssertion = m_pCompiler->optGetAssertion(assertionIndex);
 
         Limit      limit(Limit::keUndef);
-        genTreeOps cmpOper = GT_NONE;
-
-        LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lcl);
-        if (varDsc->CanBeReplacedWithItsField(m_pCompiler))
-        {
-            varDsc = m_pCompiler->lvaGetDesc(varDsc->lvFieldLclStart);
-        }
-        LclSsaVarDsc* ssaData     = varDsc->GetPerSsaData(lcl->GetSsaNum());
-        ValueNum      normalLclVN = m_pCompiler->vnStore->VNConservativeNormalValue(ssaData->m_vnPair);
+        genTreeOps cmpOper             = GT_NONE;
+        bool       isConstantAssertion = false;
 
         // Current assertion is of the form (i < len - cns) != 0
         if (curAssertion->IsCheckedBoundArithBound())
@@ -602,13 +638,20 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
             m_pCompiler->vnStore->GetCompareCheckedBound(curAssertion->op1.vn, &info);
 
             // If we don't have the same variable we are comparing against, bail.
-            if (normalLclVN != info.cmpOp)
+            if (normalLclVN == info.cmpOp)
+            {
+                cmpOper = (genTreeOps)info.cmpOper;
+                limit   = Limit(Limit::keBinOpArray, info.vnBound, 0);
+            }
+            else if (normalLclVN == info.vnBound)
+            {
+                cmpOper = GenTree::SwapRelop((genTreeOps)info.cmpOper);
+                limit   = Limit(Limit::keBinOpArray, info.cmpOp, 0);
+            }
+            else
             {
                 continue;
             }
-
-            limit   = Limit(Limit::keBinOpArray, info.vnBound, 0);
-            cmpOper = (genTreeOps)info.cmpOper;
         }
         // Current assertion is of the form (i < 100) != 0
         else if (curAssertion->IsConstantBound())
@@ -627,6 +670,36 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
             limit   = Limit(Limit::keConstant, info.constVal);
             cmpOper = (genTreeOps)info.cmpOper;
         }
+        // Current assertion is of the form i == 100
+        else if (curAssertion->IsConstantInt32Assertion())
+        {
+            if (curAssertion->op1.vn != normalLclVN)
+            {
+                continue;
+            }
+
+            int cnstLimit = m_pCompiler->vnStore->CoercedConstantValue<int>(curAssertion->op2.vn);
+
+            if ((cnstLimit == 0) && (curAssertion->assertionKind == Compiler::OAK_NOT_EQUAL) &&
+                m_pCompiler->vnStore->IsVNCheckedBound(curAssertion->op1.vn))
+            {
+                // we have arr.Len != 0, so the length must be atleast one
+                limit   = Limit(Limit::keConstant, 1);
+                cmpOper = GT_GE;
+            }
+            else if (curAssertion->assertionKind == Compiler::OAK_EQUAL)
+            {
+                limit   = Limit(Limit::keConstant, cnstLimit);
+                cmpOper = GT_EQ;
+            }
+            else
+            {
+                // We have a != assertion, but it doesn't tell us much about the interval. So just skip it.
+                continue;
+            }
+
+            isConstantAssertion = true;
+        }
         // Current assertion is not supported, ignore it
         else
         {
@@ -635,8 +708,8 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
 
         assert(limit.IsBinOpArray() || limit.IsConstant());
 
-        // Make sure the assertion is of the form != 0 or == 0.
-        if (curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT))
+        // Make sure the assertion is of the form != 0 or == 0 if it isn't a constant assertion.
+        if (!isConstantAssertion && (curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)))
         {
             continue;
         }
@@ -647,6 +720,17 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
         }
 #endif
 
+        // Limits are sometimes made with the form vn + constant, where vn is a known constant
+        // see if we can simplify this to just a constant
+        if (limit.IsBinOpArray() && m_pCompiler->vnStore->IsVNInt32Constant(limit.vn))
+        {
+            Limit tempLimit = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue<int>(limit.vn));
+            if (tempLimit.AddConstant(limit.cns))
+            {
+                limit = tempLimit;
+            }
+        }
+
         ValueNum arrLenVN = m_pCompiler->vnStore->VNConservativeNormalValue(m_pCurBndsChk->gtArrLen->gtVNPair);
 
         if (m_pCompiler->vnStore->IsVNConstant(arrLenVN))
@@ -663,22 +747,25 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
         //      (i < length) != 0
         //      (i < 100) == 0
         //      (i < 100) != 0
+        //      i == 100
         //
-        // At this point, we have detected that op1.vn is (i < length) or (i < length + cns) or
-        // (i < 100) and the op2.vn is 0.
+        // At this point, we have detected that either op1.vn is (i < length) or (i < length + cns) or
+        // (i < 100) and the op2.vn is 0 or that op1.vn is i and op2.vn is a known constant.
         //
         // Now, let us check if we are == 0 (i.e., op1 assertion is false) or != 0 (op1 assertion
-        // is true.),
+        // is true.).
         //
-        // If we have an assertion of the form == 0 (i.e., equals false), then reverse relop.
+        // If we have a non-constant assertion of the form == 0 (i.e., equals false), then reverse relop.
         // The relop has to be reversed because we have: (i < length) is false which is the same
         // as (i >= length).
-        if (curAssertion->assertionKind == Compiler::OAK_EQUAL)
+        if ((curAssertion->assertionKind == Compiler::OAK_EQUAL) && !isConstantAssertion)
         {
             cmpOper = GenTree::ReverseRelop(cmpOper);
         }
 
-        // Bounds are inclusive, so add -1 for upper bound when "<". But make sure we won't overflow.
+        assert(cmpOper != GT_NONE);
+
+        // Bounds are inclusive, so add -1 for upper bound when "<". But make sure we won't underflow.
         if (cmpOper == GT_LT && !limit.AddConstant(-1))
         {
             continue;
@@ -744,6 +831,11 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
                 pRange->lLimit = limit;
                 break;
 
+            case GT_EQ:
+                pRange->uLimit = limit;
+                pRange->lLimit = limit;
+                break;
+
             default:
                 // All other 'cmpOper' kinds leave lLimit/uLimit unchanged
                 break;