JIT: Add support for bounds check no throw assertions in range check and make overflow check more precise

src/coreclr/jit/assertionprop.cpp

@@ -769,9 +769,9 @@ void Compiler::optPrintAssertion(AssertionDsc* curAssertion, AssertionIndex asse
     }
     else if (curAssertion->op1.kind == O1K_ARR_BND)
     {
-        printf("[idx:");
+        printf("[idx: " FMT_VN, curAssertion->op1.bnd.vnIdx);
         vnStore->vnDump(this, curAssertion->op1.bnd.vnIdx);
-        printf(";len:");
+        printf("; len: " FMT_VN, curAssertion->op1.bnd.vnLen);
         vnStore->vnDump(this, curAssertion->op1.bnd.vnLen);
         printf("]");
     }

src/coreclr/jit/rangecheck.cpp

@@ -353,7 +353,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
         return;
     }
 
-    if (DoesOverflow(block, treeIndex))
+    if (DoesOverflow(block, treeIndex, range))
     {
         JITDUMP("Method determined to overflow.\n");
         return;
@@ -773,6 +773,22 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
 
             isConstantAssertion = true;
         }
+        // Current assertion asserts a bounds check does not throw
+        else if (curAssertion->IsBoundsCheckNoThrow())
+        {
+            ValueNum indexVN = curAssertion->op1.bnd.vnIdx;
+            ValueNum lenVN   = curAssertion->op1.bnd.vnLen;
+            if (normalLclVN == indexVN)
+            {
+                isUnsigned = true;
+                cmpOper    = GT_LT;
+                limit      = Limit(Limit::keBinOpArray, lenVN, 0);
+            }
+            else
+            {
+                continue;
+            }
+        }
         // Current assertion is not supported, ignore it
         else
         {
@@ -782,7 +798,8 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
         assert(limit.IsBinOpArray() || limit.IsConstant());
 
         // 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)))
+        if (!isConstantAssertion && (curAssertion->assertionKind != Compiler::OAK_NO_THROW) &&
+            (curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)))
         {
             continue;
         }
@@ -1235,17 +1252,17 @@ bool RangeCheck::MultiplyOverflows(Limit& limit1, Limit& limit2)
 }
 
 // Does the bin operation overflow.
-bool RangeCheck::DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop)
+bool RangeCheck::DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop, const Range& range)
 {
     GenTree* op1 = binop->gtGetOp1();
     GenTree* op2 = binop->gtGetOp2();
 
-    if (!m_pSearchPath->Lookup(op1) && DoesOverflow(block, op1))
+    if (!m_pSearchPath->Lookup(op1) && DoesOverflow(block, op1, range))
     {
         return true;
     }
 
-    if (!m_pSearchPath->Lookup(op2) && DoesOverflow(block, op2))
+    if (!m_pSearchPath->Lookup(op2) && DoesOverflow(block, op2, range))
     {
         return true;
     }
@@ -1279,7 +1296,7 @@ bool RangeCheck::DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop)
 }
 
 // Check if the var definition the rhs involves arithmetic that overflows.
-bool RangeCheck::DoesVarDefOverflow(GenTreeLclVarCommon* lcl)
+bool RangeCheck::DoesVarDefOverflow(BasicBlock* block, GenTreeLclVarCommon* lcl, const Range& range)
 {
     LclSsaVarDsc* ssaDef = GetSsaDefStore(lcl);
     if (ssaDef == nullptr)
@@ -1291,10 +1308,25 @@ bool RangeCheck::DoesVarDefOverflow(GenTreeLclVarCommon* lcl)
         }
         return true;
     }
-    return DoesOverflow(ssaDef->GetBlock(), ssaDef->GetDefNode()->Data());
+
+    // We can use intermediate assertions about the local to prove that any
+    // overflow on this path does not matter for the range computed.
+    Range assertionRange = Range(Limit(Limit::keUnknown));
+    MergeAssertion(block, lcl, &assertionRange DEBUGARG(0));
+
+    // But only if the range from the assertion is more strict than the global
+    // range computed; otherwise we might still have used the def's value to
+    // tighten the range of the global range.
+    Range merged = RangeOps::Merge(range, assertionRange, false);
+    if (merged.LowerLimit().Equals(range.LowerLimit()) && merged.UpperLimit().Equals(range.UpperLimit()))
+    {
+        return false;
+    }
+
+    return DoesOverflow(ssaDef->GetBlock(), ssaDef->GetDefNode()->Data(), range);
 }
 
-bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr)
+bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr, const Range& range)
 {
     for (GenTreePhi::Use& use : expr->AsPhi()->Uses())
     {
@@ -1303,25 +1335,38 @@ bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr)
         {
             continue;
         }
-        if (DoesOverflow(block, arg))
+        if (DoesOverflow(block, arg, range))
         {
             return true;
         }
     }
     return false;
 }
 
-bool RangeCheck::DoesOverflow(BasicBlock* block, GenTree* expr)
+//------------------------------------------------------------------------
+// DoesOverflow: Check if the computation of "expr" may have overflowed.
+//
+// Arguments:
+//   block - the block that contains `expr`
+//   expr  - expression to check overflow of
+//   range - range that we believe "expr" to be in without accounting for
+//           overflow; used to ignore potential overflow on paths where
+//           we can prove the value is in this range regardless.
+//
+// Return value:
+//   True if the computation may have involved an impactful overflow.
+//
+bool RangeCheck::DoesOverflow(BasicBlock* block, GenTree* expr, const Range& range)
 {
     bool overflows = false;
     if (!GetOverflowMap()->Lookup(expr, &overflows))
     {
-        overflows = ComputeDoesOverflow(block, expr);
+        overflows = ComputeDoesOverflow(block, expr, range);
     }
     return overflows;
 }
 
-bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
+bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Range& range)
 {
     JITDUMP("Does overflow [%06d]?\n", Compiler::dspTreeID(expr));
     m_pSearchPath->Set(expr, block, SearchPath::Overwrite);
@@ -1343,17 +1388,17 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
     }
     else if (expr->OperIs(GT_COMMA))
     {
-        overflows = ComputeDoesOverflow(block, expr->gtEffectiveVal());
+        overflows = ComputeDoesOverflow(block, expr->gtEffectiveVal(), range);
     }
     // Check if the var def has rhs involving arithmetic that overflows.
     else if (expr->IsLocal())
     {
-        overflows = DoesVarDefOverflow(expr->AsLclVarCommon());
+        overflows = DoesVarDefOverflow(block, expr->AsLclVarCommon(), range);
     }
     // Check if add overflows.
     else if (expr->OperIs(GT_ADD, GT_MUL))
     {
-        overflows = DoesBinOpOverflow(block, expr->AsOp());
+        overflows = DoesBinOpOverflow(block, expr->AsOp(), range);
     }
     // These operators don't overflow.
     // Actually, GT_LSH can overflow so it depends on the analysis done in ComputeRangeForBinOp
@@ -1364,11 +1409,11 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
     // Walk through phi arguments to check if phi arguments involve arithmetic that overflows.
     else if (expr->OperIs(GT_PHI))
     {
-        overflows = DoesPhiOverflow(block, expr);
+        overflows = DoesPhiOverflow(block, expr, range);
     }
     else if (expr->OperIs(GT_CAST))
     {
-        overflows = ComputeDoesOverflow(block, expr->gtGetOp1());
+        overflows = ComputeDoesOverflow(block, expr->gtGetOp1(), range);
     }
     GetOverflowMap()->Set(expr, overflows, OverflowMap::Overwrite);
     m_pSearchPath->Remove(expr);

src/coreclr/jit/rangecheck.h

@@ -199,7 +199,7 @@ struct Limit
         return false;
     }
 
-    bool Equals(Limit& l)
+    bool Equals(const Limit& l) const
     {
         switch (type)
         {
@@ -262,11 +262,21 @@ struct Range
     {
     }
 
+    const Limit& UpperLimit() const
+    {
+        return uLimit;
+    }
+
     Limit& UpperLimit()
     {
         return uLimit;
     }
 
+    const Limit& LowerLimit() const
+    {
+        return lLimit;
+    }
+
     Limit& LowerLimit()
     {
         return lLimit;
@@ -440,12 +450,12 @@ struct RangeOps
 
     // Given two ranges "r1" and "r2", do a Phi merge. If "monIncreasing" is true,
     // then ignore the dependent variables for the lower bound but not for the upper bound.
-    static Range Merge(Range& r1, Range& r2, bool monIncreasing)
+    static Range Merge(const Range& r1, const Range& r2, bool monIncreasing)
     {
-        Limit& r1lo = r1.LowerLimit();
-        Limit& r1hi = r1.UpperLimit();
-        Limit& r2lo = r2.LowerLimit();
-        Limit& r2hi = r2.UpperLimit();
+        const Limit& r1lo = r1.LowerLimit();
+        const Limit& r1hi = r1.UpperLimit();
+        const Limit& r2lo = r2.LowerLimit();
+        const Limit& r2hi = r2.UpperLimit();
 
         // Take care of lo part.
         Range result = Limit(Limit::keUnknown);
@@ -689,19 +699,19 @@ class RangeCheck
     bool MultiplyOverflows(Limit& limit1, Limit& limit2);
 
     // Does the binary operation between the operands overflow? Check recursively.
-    bool DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop);
+    bool DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop, const Range& range);
 
     // Do the phi operands involve a definition that could overflow?
-    bool DoesPhiOverflow(BasicBlock* block, GenTree* expr);
+    bool DoesPhiOverflow(BasicBlock* block, GenTree* expr, const Range& range);
 
     // Find the def of the "expr" local and recurse on the arguments if any of them involve a
     // calculation that overflows.
-    bool DoesVarDefOverflow(GenTreeLclVarCommon* lcl);
+    bool DoesVarDefOverflow(BasicBlock* block, GenTreeLclVarCommon* lcl, const Range& range);
 
-    bool ComputeDoesOverflow(BasicBlock* block, GenTree* expr);
+    bool ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Range& range);
 
-    // Does the current "expr" which is a use involve a definition, that overflows.
-    bool DoesOverflow(BasicBlock* block, GenTree* tree);
+    // Does the current "expr", which is a use, involve a definition that overflows.
+    bool DoesOverflow(BasicBlock* block, GenTree* tree, const Range& range);
 
     // Widen the range by first checking if the induction variable is monotonically increasing.
     // Requires "pRange" to be partially computed.

src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs

@@ -418,7 +418,6 @@ internal ref TValue FindValue(TKey key)
                     i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional.
                     do
                     {
-                        // Should be a while loop https://github.com/dotnet/runtime/issues/9422
                         // Test in if to drop range check for following array access
                         if ((uint)i >= (uint)entries.Length)
                         {
@@ -450,7 +449,6 @@ internal ref TValue FindValue(TKey key)
                     i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional.
                     do
                     {
-                        // Should be a while loop https://github.com/dotnet/runtime/issues/9422
                         // Test in if to drop range check for following array access
                         if ((uint)i >= (uint)entries.Length)
                         {
@@ -535,15 +533,8 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
                 comparer == null)
             {
                 // ValueType: Devirtualize with EqualityComparer<TKey>.Default intrinsic
-                while (true)
+                while ((uint)i < (uint)entries.Length)
                 {
-                    // Should be a while loop https://github.com/dotnet/runtime/issues/9422
-                    // Test uint in if rather than loop condition to drop range check for following array access
-                    if ((uint)i >= (uint)entries.Length)
-                    {
-                        break;
-                    }
-
                     if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))
                     {
                         if (behavior == InsertionBehavior.OverwriteExisting)
@@ -574,15 +565,8 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
             else
             {
                 Debug.Assert(comparer is not null);
-                while (true)
+                while ((uint)i < (uint)entries.Length)
                 {
-                    // Should be a while loop https://github.com/dotnet/runtime/issues/9422
-                    // Test uint in if rather than loop condition to drop range check for following array access
-                    if ((uint)i >= (uint)entries.Length)
-                    {
-                        break;
-                    }
-
                     if (entries[i].hashCode == hashCode && comparer.Equals(entries[i].key, key))
                     {
                         if (behavior == InsertionBehavior.OverwriteExisting)
@@ -690,15 +674,8 @@ internal static class CollectionsMarshalHelper
                     comparer == null)
                 {
                     // ValueType: Devirtualize with EqualityComparer<TKey>.Default intrinsic
-                    while (true)
+                    while ((uint)i < (uint)entries.Length)
                     {
-                        // Should be a while loop https://github.com/dotnet/runtime/issues/9422
-                        // Test uint in if rather than loop condition to drop range check for following array access
-                        if ((uint)i >= (uint)entries.Length)
-                        {
-                            break;
-                        }
-
                         if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))
                         {
                             exists = true;
@@ -720,15 +697,8 @@ internal static class CollectionsMarshalHelper
                 else
                 {
                     Debug.Assert(comparer is not null);
-                    while (true)
+                    while ((uint)i < (uint)entries.Length)
                     {
-                        // Should be a while loop https://github.com/dotnet/runtime/issues/9422
-                        // Test uint in if rather than loop condition to drop range check for following array access
-                        if ((uint)i >= (uint)entries.Length)
-                        {
-                            break;
-                        }
-
                         if (entries[i].hashCode == hashCode && comparer.Equals(entries[i].key, key))
                         {
                             exists = true;