From 622c8e4bd0cc5ee72d0fab9ecacd61ca77d28c6a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 19 Jul 2024 23:50:57 +0200 Subject: [PATCH] JIT: Switch `StaysWithinManagedObject` to peel offsets from VNs The SCEV analysis does not care about the value of something once it is seen to be invariant inside the loop we are currently analyzing. This was problematic for this logic that tries to peel additions away from offsets; for arm64, we may have hoisted `array + 0x10` outside the loop, which would cause us to fail to get back to the base array. Switch the reasoning to use VNs and peel the offsets from the VNs instead. No x64 diffs are expected as we do not hoist the `array + 0x10` out of the loop there. Improvements expected on arm64 where we can now prove that a "full" strength reduction is allowable more often. --- src/coreclr/jit/inductionvariableopts.cpp | 50 +++++++++++++---------- src/coreclr/jit/valuenum.cpp | 36 ++++++++++++++++ src/coreclr/jit/valuenum.h | 2 + 3 files changed, 67 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/inductionvariableopts.cpp b/src/coreclr/jit/inductionvariableopts.cpp index 32c5a1967b6b05..fa3d627feadcc8 100644 --- a/src/coreclr/jit/inductionvariableopts.cpp +++ b/src/coreclr/jit/inductionvariableopts.cpp @@ -1845,16 +1845,32 @@ bool StrengthReductionContext::CheckAdvancedCursors(ArrayStack* curs // bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack* cursors, ScevAddRec* addRec) { - int64_t offset; - Scev* baseScev = addRec->Start->PeelAdditions(&offset); - offset = static_cast(offset); + ValueNumPair addRecStartVNP = m_scevContext.MaterializeVN(addRec->Start); + if (!addRecStartVNP.BothDefined()) + { + return false; + } + + ValueNumPair addRecStartBase = addRecStartVNP; + target_ssize_t offsetLiberal = 0; + target_ssize_t offsetConservative = 0; + m_comp->vnStore->PeelOffsets(addRecStartBase.GetLiberalAddr(), &offsetLiberal); + m_comp->vnStore->PeelOffsets(addRecStartBase.GetConservativeAddr(), &offsetConservative); + + if (offsetLiberal != offsetConservative) + { + return false; + } + + target_ssize_t offset = offsetLiberal; - // 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)) + // We only support objects here (targeting array/strings). To strength + // reduce Span accesses we need additional properties 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 ((m_comp->vnStore->TypeOfVN(addRecStartBase.GetConservative()) != TYP_REF) || + (m_comp->vnStore->TypeOfVN(addRecStartBase.GetLiberal()) != TYP_REF)) { return false; } @@ -1888,22 +1904,14 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack* return false; } - ScevLocal* local = (ScevLocal*)baseScev; - - ValueNumPair vnp = m_scevContext.MaterializeVN(baseScev); - if (!vnp.BothDefined()) - { - return false; - } - BasicBlock* preheader = m_loop->EntryEdge(0)->getSourceBlock(); - if (!m_comp->optAssertionVNIsNonNull(vnp.GetConservative(), preheader->bbAssertionOut)) + if (!m_comp->optAssertionVNIsNonNull(addRecStartBase.GetConservative(), preheader->bbAssertionOut)) { return false; } - // 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 + // We have a non-null object as the base. 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? @@ -1914,7 +1922,7 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack* // 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()); + ValueNum arrLengthVN = m_comp->vnStore->VNForFunc(TYP_INT, VNF_ARR_LENGTH, addRecStartBase.GetLiberal()); for (int i = 0; i < m_backEdgeBounds.Height(); i++) { diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 4d5ad347744a03..7e00b61aabb25b 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -14998,3 +14998,39 @@ CORINFO_CLASS_HANDLE ValueNumStore::GetObjectType(ValueNum vn, bool* pIsExact, b return NO_CLASS_HANDLE; } + +//-------------------------------------------------------------------------------- +// PeelOffsets: Peel all additions with a constant offset away from the +// specified VN. +// +// Arguments: +// vn - [in, out] The VN. Will be modified to the base VN that the offsets are added to. +// offset - [out] The offsets peeled out of the VNF_ADD funcs. +// +void ValueNumStore::PeelOffsets(ValueNum* vn, target_ssize_t* offset) +{ +#ifdef DEBUG + var_types vnType = TypeOfVN(*vn); + assert((vnType == TYP_I_IMPL) || (vnType == TYP_REF) || (vnType == TYP_BYREF)); +#endif + + *offset = 0; + VNFuncApp app; + while (GetVNFunc(*vn, &app) && (app.m_func == VNF_ADD)) + { + if (IsVNConstantNonHandle(app.m_args[0])) + { + *offset += ConstantValue(app.m_args[0]); + *vn = app.m_args[1]; + } + else if (IsVNConstantNonHandle(app.m_args[1])) + { + *offset += ConstantValue(app.m_args[1]); + *vn = app.m_args[0]; + } + else + { + break; + } + } +} diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 0138e6e5aecf7c..b9592d8c8cae27 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -512,6 +512,8 @@ class ValueNumStore CORINFO_CLASS_HANDLE GetObjectType(ValueNum vn, bool* pIsExact, bool* pIsNonNull); + void PeelOffsets(ValueNum* vn, target_ssize_t* offset); + // And the single constant for an object reference type. static ValueNum VNForNull() {