JIT: Improve and fix StaysWithinManagedObject (#105108)

- For string accesses we also produce `ARR_ADDR`, so we must take care
  to use `GenTreeArrAddr::GetFirstElemOffset` instead of hardcoding
  `OFFSETOF__CORINFO_Array__data`
- There are cases where VN is fully able to prove that bound <
  ARR_LENGTH(vn), specifically when the array is stored in a static
  readonly field. In those cases everything reduces to constants, so
  allow VN to try to prove it but fall back to our manual logic
  otherwise.
- Rephrase the fallback as a VN test as well. In a standard `for (;i <
  arr.Length;)` loop we have a bound on the backedge of the shape
  `ARR_LENGTH(array) - 1`. The previous strategy was to syntactically
  check if the LHS was such an array length on the same array as the
  base of the add recurrence.

  Instead of doing that, we can ask more generally for any shape `x - c`
  whether we know that `x <= ARR_LENGTH(array)`. In the usual case of `x
  == ARR_LENGTH(array)` this is trivially true and VN knows that.
  However, there are other cases where this is provable by RBO due to a
  dominating compare; particularly loop cloning introduces these
  dominating compares when cloning loops of the shape `for (; i < n;)`.
  This fixes #105087.

Author: jakobbotsch

src/coreclr/jit/inductionvariableopts.cpp

@@ -1849,17 +1849,18 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack<CursorInfo>*
     Scev*   baseScev = addRec->Start->PeelAdditions(&offset);
     offset           = static_cast<target_ssize_t>(offset);
 
-    // We only support arrays here. To strength reduce Span<T> accesses we need
-    // additional properies on the range designated by a Span<T> that we
-    // currently do not specify, or we need to prove that the byref we may form
-    // in the IV update would have been formed anyway by the loop.
+    // We only support arrays and strings here. To strength reduce Span<T>
+    // accesses we need additional properies on the range designated by a
+    // Span<T> that we currently do not specify, or we need to prove that the
+    // byref we may form in the IV update would have been formed anyway by the
+    // loop.
     if (!baseScev->OperIs(ScevOper::Local) || !baseScev->TypeIs(TYP_REF))
     {
         return false;
     }
 
-    // Now use the fact that we keep ARR_ADDRs in the IR when we have array
-    // accesses.
+    // Now use the fact that we keep ARR_ADDRs in the IR when we have
+    // array/string accesses.
     GenTreeArrAddr* arrAddr = nullptr;
     for (int i = 0; i < cursors->Height(); i++)
     {
@@ -1890,7 +1891,7 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack<CursorInfo>*
     ScevLocal* local = (ScevLocal*)baseScev;
 
     ValueNumPair vnp = m_scevContext.MaterializeVN(baseScev);
-    if (vnp.GetConservative() == ValueNumStore::NoVN)
+    if (!vnp.BothDefined())
     {
         return false;
     }
@@ -1901,53 +1902,68 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack<CursorInfo>*
         return false;
     }
 
-    // We have a non-null array. Check that the 'start' offset looks fine.
-    // TODO: We could also use assertions on the length of the array. E.g. if
-    // we know the length of the array is > 3, then we can allow the add rec to
-    // have a later start. Maybe range check can be used?
-    if ((offset < 0) || (offset > (int64_t)OFFSETOF__CORINFO_Array__data))
+    // We have a non-null array/string. Check that the 'start' offset looks
+    // fine. TODO: We could also use assertions on the length of the
+    // array/string. E.g. if we know the length of the array is > 3, then we
+    // can allow the add rec to have a later start. Maybe range check can be
+    // used?
+    if ((offset < 0) || (offset > arrAddr->GetFirstElemOffset()))
     {
         return false;
     }
 
-    // Now see if we have a bound that guarantees that we iterate less than the
-    // array length's times.
+    // Now see if we have a bound that guarantees that we iterate fewer times
+    // than the array/string's length.
+    ValueNum arrLengthVN = m_comp->vnStore->VNForFunc(TYP_INT, VNF_ARR_LENGTH, vnp.GetLiberal());
+
     for (int i = 0; i < m_backEdgeBounds.Height(); i++)
     {
-        // TODO: EvaluateRelop ought to be powerful enough to prove something
-        // like bound < ARR_LENGTH(vn), but it is not able to prove that
-        // currently, even for bound = ARR_LENGTH(vn) - 1 (common case).
         Scev* bound = m_backEdgeBounds.Bottom(i);
+        if (!bound->TypeIs(TYP_INT))
+        {
+            // Currently cannot handle bounds that aren't 32 bit.
+            continue;
+        }
 
-        int64_t boundOffset;
-        Scev*   boundBase = bound->PeelAdditions(&boundOffset);
-
-        if (bound->TypeIs(TYP_INT))
+        // In some cases VN is powerful enough to evaluate bound <
+        // ARR_LENGTH(vn) directly.
+        ValueNumPair boundVN = m_scevContext.MaterializeVN(bound);
+        if (boundVN.GetLiberal() != ValueNumStore::NoVN)
         {
-            boundOffset = static_cast<int32_t>(boundOffset);
+            ValueNum relop = m_comp->vnStore->VNForFunc(TYP_INT, VNF_LT_UN, boundVN.GetLiberal(), arrLengthVN);
+            if (m_scevContext.EvaluateRelop(relop) == RelopEvaluationResult::True)
+            {
+                return true;
+            }
         }
 
+        // In common cases VN cannot prove the above, so try a little bit
+        // harder. In this case we know the bound doesn't overflow
+        // (conceptually the bound is an arbitrary precision integer and a
+        // negative bound does not make sense).
+        int64_t boundOffset;
+        Scev*   boundBase = bound->PeelAdditions(&boundOffset);
+
+        boundOffset = static_cast<int32_t>(boundOffset);
         if (boundOffset >= 0)
         {
             // If we take the backedge >= the array length times, then we would
             // advance the addrec past the end.
             continue;
         }
 
+        // Now try to prove that boundBase <= ARR_LENGTH(vn). Commonly
+        // boundBase == ARR_LENGTH(vn) exactly, but it may also have a
+        // dominating subsuming compare before the loop due to loop cloning.
         ValueNumPair boundBaseVN = m_scevContext.MaterializeVN(boundBase);
-
-        VNFuncApp vnf;
-        if (!m_comp->vnStore->GetVNFunc(boundBaseVN.GetConservative(), &vnf))
-        {
-            continue;
-        }
-
-        if ((vnf.m_func != VNF_ARR_LENGTH) || (vnf.m_args[0] != vnp.GetConservative()))
+        if (boundBaseVN.GetLiberal() != ValueNumStore::NoVN)
         {
-            continue;
+            ValueNum relop = m_comp->vnStore->VNForFunc(TYP_INT, VNF_LE, boundBaseVN.GetLiberal(), arrLengthVN);
+            if (m_scevContext.EvaluateRelop(relop) == RelopEvaluationResult::True)
+            {
+                return true;
+            }
         }
-
-        return true;
     }
 
     return false;

src/coreclr/jit/scev.cpp

@@ -1501,16 +1501,13 @@ RelopEvaluationResult ScalarEvolutionContext::EvaluateRelop(ValueNum vn)
     }
 
     // Evaluate by using dominators and RBO's logic.
+    assert(m_comp->m_domTree != nullptr);
     //
     // TODO-CQ: Using assertions could be stronger given its dataflow, but it
     // is not convenient to use (optVNConstantPropOnJTrue does not actually
     // make any use of assertions to evaluate conditionals, so it seems like
     // the logic does not actually exist anywhere.)
     //
-    if (m_comp->m_domTree == nullptr)
-    {
-        m_comp->m_domTree = FlowGraphDominatorTree::Build(m_comp->m_dfsTree);
-    }
 
     for (BasicBlock* idom = m_loop->GetHeader()->bbIDom; idom != nullptr; idom = idom->bbIDom)
     {

src/coreclr/jit/scev.h

@@ -239,10 +239,9 @@ class ScalarEvolutionContext
     Scev* CreateScevForConstant(GenTreeIntConCommon* tree);
     void  ExtractAddOperands(ScevBinop* add, ArrayStack<Scev*>& operands);
 
-    VNFunc                MapRelopToVNFunc(genTreeOps oper, bool isUnsigned);
-    RelopEvaluationResult EvaluateRelop(ValueNum relop);
-    bool                  MayOverflowBeforeExit(ScevAddRec* lhs, Scev* rhs, VNFunc exitOp);
-    bool AddRecMayOverflow(ScevAddRec* addRec, bool signedBound, const SimplificationAssumptions& assumptions);
+    VNFunc MapRelopToVNFunc(genTreeOps oper, bool isUnsigned);
+    bool   MayOverflowBeforeExit(ScevAddRec* lhs, Scev* rhs, VNFunc exitOp);
+    bool   AddRecMayOverflow(ScevAddRec* addRec, bool signedBound, const SimplificationAssumptions& assumptions);
 
     bool Materialize(Scev* scev, bool createIR, GenTree** result, ValueNumPair* resultVN);
 
@@ -262,7 +261,8 @@ class ScalarEvolutionContext
     static const SimplificationAssumptions NoAssumptions;
     Scev* Simplify(Scev* scev, const SimplificationAssumptions& assumptions = NoAssumptions);
 
-    Scev* ComputeExitNotTakenCount(BasicBlock* exiting);
+    Scev*                 ComputeExitNotTakenCount(BasicBlock* exiting);
+    RelopEvaluationResult EvaluateRelop(ValueNum relop);
 
     GenTree*     Materialize(Scev* scev);
     ValueNumPair MaterializeVN(Scev* scev);