Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Switch StaysWithinManagedObject to peel offsets from VNs #105169

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 29 additions & 21 deletions src/coreclr/jit/inductionvariableopts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1845,16 +1845,32 @@ bool StrengthReductionContext::CheckAdvancedCursors(ArrayStack<CursorInfo>* curs
//
bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack<CursorInfo>* cursors, ScevAddRec* addRec)
{
int64_t offset;
Scev* baseScev = addRec->Start->PeelAdditions(&offset);
offset = static_cast<target_ssize_t>(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<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))
// We only support objects here (targeting array/strings). To strength
// reduce Span<T> accesses we need additional properties 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 ((m_comp->vnStore->TypeOfVN(addRecStartBase.GetConservative()) != TYP_REF) ||
(m_comp->vnStore->TypeOfVN(addRecStartBase.GetLiberal()) != TYP_REF))
{
return false;
}
Expand Down Expand Up @@ -1888,22 +1904,14 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack<CursorInfo>*
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?
Expand All @@ -1914,7 +1922,7 @@ bool StrengthReductionContext::StaysWithinManagedObject(ArrayStack<CursorInfo>*

// 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++)
{
Expand Down
36 changes: 36 additions & 0 deletions src/coreclr/jit/valuenum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like VN ought to commute so that constants are (say) rightmost / and or fold chains of constant adds... but I take it you're seeing add chains or mixtures of constants left and right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I haven't checked whether we see cases that require the extra unwrapping... I just wrote this one fully generally to match gtPeelOffsets and Scev::PeelAdditions.
We do canonicalize commutative operators, but only in terms of raw VN number, keeping the smallest VN number as op1. Definitely seems like we could make sure constants go to the right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely seems like we could make sure constants go to the right.

I remember I tried that but found no diffs

{
if (IsVNConstantNonHandle(app.m_args[0]))
{
*offset += ConstantValue<target_ssize_t>(app.m_args[0]);
*vn = app.m_args[1];
}
else if (IsVNConstantNonHandle(app.m_args[1]))
{
*offset += ConstantValue<target_ssize_t>(app.m_args[1]);
*vn = app.m_args[0];
}
else
{
break;
}
}
}
2 changes: 2 additions & 0 deletions src/coreclr/jit/valuenum.h
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
Loading