From fb2d27ab5e8b80ce896e9df339b8d897ab4aaaf8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 18 Jul 2024 15:58:49 +0200 Subject: [PATCH] JIT: Improve and fix `StaysWithinManagedObject` - 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. --- src/coreclr/jit/inductionvariableopts.cpp | 82 ++++++++++++++--------- src/coreclr/jit/scev.cpp | 5 +- src/coreclr/jit/scev.h | 10 +-- 3 files changed, 55 insertions(+), 42 deletions(-) diff --git a/src/coreclr/jit/inductionvariableopts.cpp b/src/coreclr/jit/inductionvariableopts.cpp index 9ae98dfbf7770d..f57c663f8ad342 100644 --- a/src/coreclr/jit/inductionvariableopts.cpp +++ b/src/coreclr/jit/inductionvariableopts.cpp @@ -1819,17 +1819,18 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack* Scev* baseScev = addRec->Start->PeelAdditions(&offset); offset = static_cast(offset); - // We only support arrays here. To strength reduce Span accesses we need - // additional properies on the range designated by a Span 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 + // accesses we need additional properies on the range designated by a + // Span 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++) { @@ -1860,7 +1861,7 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack* ScevLocal* local = (ScevLocal*)baseScev; ValueNumPair vnp = m_scevContext.MaterializeVN(baseScev); - if (vnp.GetConservative() == ValueNumStore::NoVN) + if (!vnp.BothDefined()) { return false; } @@ -1871,32 +1872,49 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack* 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(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(boundOffset); if (boundOffset >= 0) { // If we take the backedge >= the array length times, then we would @@ -1904,20 +1922,18 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack* 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; diff --git a/src/coreclr/jit/scev.cpp b/src/coreclr/jit/scev.cpp index ec7d8febb31507..540d5bed9f0e7f 100644 --- a/src/coreclr/jit/scev.cpp +++ b/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) { diff --git a/src/coreclr/jit/scev.h b/src/coreclr/jit/scev.h index 1714d1d3fc3152..654a450482086a 100644 --- a/src/coreclr/jit/scev.h +++ b/src/coreclr/jit/scev.h @@ -239,10 +239,9 @@ class ScalarEvolutionContext Scev* CreateScevForConstant(GenTreeIntConCommon* tree); void ExtractAddOperands(ScevBinop* add, ArrayStack& 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);