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.

Sign up for GitHub

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

Update the JIT to track Span.Length and ReadOnlySpan.Length as "never negative" #81055

Merged
merged 9 commits into from
Jan 26, 2023
Merged

Update the JIT to track Span.Length and ReadOnlySpan.Length as "never negative" #81055

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5dac74f
Update the JIT to track `Span.Length` and `ReadOnlySpan.Length` as "n…
tannergooding Jan 23, 2023
c88ae01
Updating the "is never negative" info to be tracked in LclVarDsc
tannergooding Jan 24, 2023
820a5d8
Apply formatting patch
tannergooding Jan 24, 2023
618226e
Ensure lvIsNeverNegative is propagated to shadows
tannergooding Jan 24, 2023
e5d4b13
Update src/coreclr/jit/gentree.h
tannergooding Jan 25, 2023
d700615
Responding to PR feedback
tannergooding Jan 25, 2023
60ffb5a
Adding two asserts
tannergooding Jan 25, 2023
2f882d8
Rename GenTreeField::IsNeverNegative to IsSpanLength and add a comment
tannergooding Jan 25, 2023
c224a3d
Fix an assert
tannergooding Jan 25, 2023
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
20 changes: 19 additions & 1 deletion src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,20 @@ bool IntegralRange::Contains(int64_t value) const
break;

case GT_LCL_VAR:
if (compiler->lvaGetDesc(node->AsLclVar())->lvNormalizeOnStore())
{
LclVarDsc* const varDsc = compiler->lvaGetDesc(node->AsLclVar());

if (varDsc->lvNormalizeOnStore())
{
rangeType = compiler->lvaGetDesc(node->AsLclVar())->TypeGet();
}

if (varDsc->IsNeverNegative())
{
return {SymbolicIntegerValue::Zero, UpperBoundForType(rangeType)};
}
break;
}

case GT_CNS_INT:
if (node->IsIntegralConst(0) || node->IsIntegralConst(1))
Expand Down Expand Up @@ -218,6 +227,15 @@ bool IntegralRange::Contains(int64_t value) const
break;
#endif // defined(FEATURE_HW_INTRINSICS)

case GT_FIELD:
{
if (node->AsField()->IsNeverNegative())
{
return {SymbolicIntegerValue::Zero, UpperBoundForType(rangeType)};
}
break;
}

default:
break;
}
Expand Down
16 changes: 16 additions & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,10 @@ class LclVarDsc
unsigned char lvIsOSRLocal : 1; // Root method local in an OSR method. Any stack home will be on the Tier0 frame.
// Initial value will be defined by Tier0. Requires special handing in prolog.

private:
unsigned char lvIsNeverNegative : 1; // The local is known to be never negative

public:
union {
unsigned lvFieldLclStart; // The index of the local var representing the first field in the promoted struct
// local. For implicit byref parameters, this gets hijacked between
Expand Down Expand Up @@ -958,6 +962,18 @@ class LclVarDsc
}
#endif

// Is this is local never negative?
bool IsNeverNegative() const
{
return lvIsNeverNegative;
}

// Is this is local never negative?
void SetIsNeverNegative(bool value)
{
lvIsNeverNegative = value;
}

/////////////////////

regNumber GetArgInitReg() const
Expand Down
46 changes: 46 additions & 0 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8433,6 +8433,11 @@ GenTree* Compiler::gtClone(GenTree* tree, bool complexOK)
#ifdef FEATURE_READYTORUN
copy->AsField()->gtFieldLookup = tree->AsField()->gtFieldLookup;
#endif

if (tree->AsField()->IsNeverNegative())
{
copy->AsField()->SetIsNeverNegative(true);
}
}
else if (tree->OperIs(GT_ADD, GT_SUB))
{
Expand Down Expand Up @@ -8628,13 +8633,17 @@ GenTree* Compiler::gtCloneExpr(
goto DONE;

case GT_LCL_VAR_ADDR:
{
copy = new (this, oper) GenTreeLclVar(oper, tree->TypeGet(), tree->AsLclVar()->GetLclNum());
goto DONE;
}

case GT_LCL_FLD_ADDR:
{
copy = new (this, oper)
GenTreeLclFld(oper, tree->TypeGet(), tree->AsLclFld()->GetLclNum(), tree->AsLclFld()->GetLclOffs());
goto DONE;
}

default:
NO_WAY("Cloning of node not supported");
Expand Down Expand Up @@ -8772,6 +8781,11 @@ GenTree* Compiler::gtCloneExpr(
#ifdef FEATURE_READYTORUN
copy->AsField()->gtFieldLookup = tree->AsField()->gtFieldLookup;
#endif

if (tree->AsField()->IsNeverNegative())
{
copy->AsField()->SetIsNeverNegative(true);
}
break;

case GT_BOX:
Expand Down Expand Up @@ -24618,6 +24632,20 @@ ClassLayout* GenTreeLclVarCommon::GetLayout(Compiler* compiler) const
return AsLclFld()->GetLayout();
}

//------------------------------------------------------------------------
// GenTreeLclVar::IsNeverNegative: Gets true if the lcl var is never negative; otherwise false.
//
// Arguments:
// comp - the compiler instance
//
// Return Value:
// true if the lcl var is never negative; otherwise false.
//
bool GenTreeLclVar::IsNeverNegative(Compiler* comp) const
{
return comp->lvaGetDesc(GetLclNum())->IsNeverNegative();
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
}

#if defined(TARGET_XARCH) && defined(FEATURE_HW_INTRINSICS)
//------------------------------------------------------------------------
// GetResultOpNumForFMA: check if the result is written into one of the operands.
Expand Down Expand Up @@ -24711,6 +24739,24 @@ bool GenTree::IsNeverNegative(Compiler* comp) const
{
return AsIntConCommon()->IntegralValue() >= 0;
}

if (OperIs(GT_LCL_VAR))
{
if (AsLclVar()->IsNeverNegative(comp))
{
// This is an early exit, it doesn't cover all cases
return true;
}
}
else if (OperIs(GT_FIELD))
{
if (AsField()->IsNeverNegative())
{
// This is an early exit, it doesn't cover all cases
return true;
}
}

// TODO-Casts: extend IntegralRange to handle constants
return IntegralRange::ForNode((GenTree*)this, comp).IsPositive();
}
25 changes: 23 additions & 2 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3794,6 +3794,8 @@ struct GenTreeLclVar : public GenTreeLclVarCommon
unsigned int GetFieldCount(Compiler* compiler) const;
var_types GetFieldTypeByIndex(Compiler* compiler, unsigned idx);

bool IsNeverNegative(Compiler* comp) const;

//-------------------------------------------------------------------
// clearOtherRegFlags: clear GTF_* flags associated with gtOtherRegs
//
Expand Down Expand Up @@ -4004,13 +4006,22 @@ struct GenTreeField : public GenTreeUnOp
{
CORINFO_FIELD_HANDLE gtFldHnd;
DWORD gtFldOffset;
bool gtFldMayOverlap;
bool gtFldMayOverlap : 1;

private:
bool gtFldIsNeverNegative : 1;

public:
#ifdef FEATURE_READYTORUN
CORINFO_CONST_LOOKUP gtFieldLookup;
#endif

GenTreeField(genTreeOps oper, var_types type, GenTree* obj, CORINFO_FIELD_HANDLE fldHnd, DWORD offs)
: GenTreeUnOp(oper, type, obj), gtFldHnd(fldHnd), gtFldOffset(offs), gtFldMayOverlap(false)
: GenTreeUnOp(oper, type, obj)
, gtFldHnd(fldHnd)
, gtFldOffset(offs)
, gtFldMayOverlap(false)
, gtFldIsNeverNegative(false)
{
#ifdef FEATURE_READYTORUN
gtFieldLookup.addr = nullptr;
Expand All @@ -4037,6 +4048,16 @@ struct GenTreeField : public GenTreeUnOp
return (gtFlags & GTF_FLD_VOLATILE) != 0;
}

bool IsNeverNegative() const
{
return gtFldIsNeverNegative;
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
}

void SetIsNeverNegative(bool value)
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
{
gtFldIsNeverNegative = value;
}

bool IsInstance() const
{
return GetFldObj() != nullptr;
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/jit/gschecks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,11 @@ void Compiler::gsParamsToShadows()
shadowVarDsc->lvIsUnsafeBuffer = varDsc->lvIsUnsafeBuffer;
shadowVarDsc->lvIsPtr = varDsc->lvIsPtr;

if (varDsc->IsNeverNegative())
{
shadowVarDsc->SetIsNeverNegative(true);
}

#ifdef DEBUG
if (verbose)
{
Expand Down
9 changes: 7 additions & 2 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2781,7 +2781,10 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
// Bounds check
CORINFO_FIELD_HANDLE lengthHnd = info.compCompHnd->getFieldInClass(clsHnd, 1);
const unsigned lengthOffset = info.compCompHnd->getFieldOffset(lengthHnd);
GenTree* length = gtNewFieldRef(TYP_INT, lengthHnd, ptrToSpan, lengthOffset);

GenTreeField* length = gtNewFieldRef(TYP_INT, lengthHnd, ptrToSpan, lengthOffset);
length->SetIsNeverNegative(true);

GenTree* boundsCheck = new (this, GT_BOUNDS_CHECK) GenTreeBoundsChk(index, length, SCK_RNGCHK_FAIL);

// Element access
Expand Down Expand Up @@ -2840,7 +2843,9 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
CORINFO_FIELD_HANDLE lengthHnd = info.compCompHnd->getFieldInClass(clsHnd, 1);
const unsigned lengthOffset = info.compCompHnd->getFieldOffset(lengthHnd);

return gtNewFieldRef(TYP_INT, lengthHnd, ptrToSpan, lengthOffset);
GenTreeField* fieldRef = gtNewFieldRef(TYP_INT, lengthHnd, ptrToSpan, lengthOffset);
fieldRef->SetIsNeverNegative(true);
return fieldRef;
}

case NI_System_RuntimeTypeHandle_GetValueInternal:
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/jit/lclmorph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1392,6 +1392,11 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
{
GenTreeFlags lclVarFlags = node->gtFlags & (GTF_NODE_MASK | GTF_DONT_CSE);

if (node->OperIs(GT_FIELD) && node->AsField()->IsNeverNegative())
{
m_compiler->lvaGetDesc(fieldLclNum)->SetIsNeverNegative(true);
Copy link
Member

@jakobbotsch jakobbotsch Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Does it make any difference in the PMI diffs if we add CORINFO_FLG_SPAN that is returned by getClassAttribs for Span<T> and ReadOnlySpan<T>, and then use it in Compiler::StructPromotionHelper::PromoteStructVar to set this bit on the second local created?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look independently, but I wouldn't expect significant diffs and I don't think we'd want this to be the exclusive way to determine that.

-- Posting a rough summary of what got discussed on Discord.

In order for user code to access the field, they must go through get_Length. They could reinterpret cast or use pointers or other things, but those are all rarer and potentially dangerous.

For the block copy case, you don't really end up with interesting opts possible. The user could then access the copied field, but that would still go through get_Length and therefore the intrinsic path.

We'd want to keep the flag we set as part of import to handle any early constant folding handling. Such optimizations generally have positive impact on TP due to the early dead code removal they allow (and therefore making the total IR to process significantly smaller from the start).

There are other potential opts and tracking we could do, including loop cloning if we had some CORINFO_FLG_SPAN and there might be some cases like multi-reg arg passing where the intrinsic path wouldn't necessarily catch things

Copy link
Member

Choose a reason for hiding this comment

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

I am still not a fan of this propagation of "never negative" from a GT_FIELD access to the LclVarDsc. It makes an implicit assumption that the local will always satisfy this condition. That is ok for the uses of IsNeverNegative() introduced by this PR but is a very surprising precondition to require in JIT IR (and in contrast to things like GTF_IND_NONNULL). I think it makes the state fragile in the future.

If we want to keep it this way I think the new accessors/information on GT_FIELD needs to be renamed into something like IsNonNegativeLengthAccess() that makes it more clear that the access is on a local that is expected not to break the invariant.

My preference would still be to introduce CORINFO_FLG_SPAN and set it on span locals. We can teach GenTree:IsNeverNegative/IntegralRange::ForNode to recognize accesses of the length (for both promoted access and potential non-promoted access, before promotion happens). It would be more in line with ARR_LENGTH today and would not require adding new state to any nodes (only a lvIsSpan on LclVarDsc that can be set as part of lvaSetStruct -- we already call getClassAttribs).

Copy link
Member Author

Choose a reason for hiding this comment

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

My preference would still be to introduce CORINFO_FLG_SPAN and set it on span locals. We can teach GenTree:IsNeverNegative/IntegralRange::ForNode to recognize accesses of the length (for both promoted access and potential non-promoted access, before promotion happens). It would be more in line with ARR_LENGTH today and would not require adding new state to any nodes (only a lvIsSpan on LclVarDsc that can be set as part of lvaSetStruct -- we already call getClassAttribs).

I'm concerned about the additional cost that doing that would bring. What we have here is simple, effectively, extensible, and works without negatively impacting throughput (it's a small sub 0.01% gain on most platforms and a small sub 0.01% regression on a couple).

While this wouldn't add overhead to the promotion case since we already call getClassAttribs, doing pre-promotion recognition would require additional JIT/EE calls to be introduced which can negatively impact throughput. We would then need to either track that information somehow or continue doing lookups against the field handle where necessary (and we cannot trivially cache since we have a different handle per T).

If we want to keep it this way I think the new accessors/information on GT_FIELD needs to be renamed into something like IsNonNegativeLengthAccess() that makes it more clear that the access is on a local that is expected not to break the invariant.

I think this is reasonable for now. That being said, I think we want and need this to be more generally extensible long term. The concept of a exposing an int but the code requiring it to be "never negative" is fairly integrated throughout the entirety of .NET. In practice it is similar to most APIs not allowing references to be null. There are a good deal of optimizations that are only valid on unsigned types and which will allow us to provide significantly better perf/codegen by recognizing. -- The main reason this diff "looks small" is because we are already explicitly casting to the equivalent unsigned type in much of our managed BCL code and so the remaining cases are places where we weren't or couldn't.

That is ok for the uses of IsNeverNegative() introduced by this PR but is a very surprising precondition to require in JIT IR (and in contrast to things like GTF_IND_NONNULL). I think it makes the state fragile in the future.

Can you elaborate on the fragility and how you think future changes could impact this?

Are you referring to locals getting tagged and then reused for something else? Users assigning negative values (which should only be possible via unsafe code)? Or something else?

Copy link
Member

Choose a reason for hiding this comment

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

While this wouldn't add overhead to the promotion case since we already call getClassAttribs, doing pre-promotion recognition would require additional JIT/EE calls to be introduced which can negatively impact throughput. We would then need to either track that information somehow or continue doing lookups against > the field handle where necessary (and we cannot trivially cache since we have a different handle per T).

The overhead would be in the pattern recognition inside GenTree::IsNeverNegative/IntegralRange::ForNode, but we would need to measure it to make any conclusions.

There would be no new EE calls since lvaSetStruct already calls getClassAttribs. It would make getClassAttribs more expensive on the EE side, but I expect we will want to do this anyway at some point in the (near) future to support loop cloning for spans.

Can you elaborate on the fragility and how you think future changes could impact this?

Are you referring to locals getting tagged and then reused for something else? Users assigning negative values (which should only be possible via unsafe code)? Or something else?

I am referring to a future JIT dev marking a GT_FIELD node as never negative because the field access is non-negative at that particular point in time, in the same way one would with GTF_IND_NONNULL. That's not legal due to this propagation into LclVarDsc that actually requires the backing storage to always be non-negative globally in the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked a bit more on Discord to get clarification.

I've updated GenTreeField::IsNeverNegative to be GenTreeField::IsSpanLength to help avoid any issues around the current propagation. There exists a comment in the method that goes into more detail around the "why".

The plan after this PR is to look into also providing the getClassAttribs support since that should allow other Array like optimizations to kick in for span. However, the intent for the support being added in this PR is that it will stick around both to help catch pre-promotion dead code elimination and because the belief is that there are more general purpose areas where the information that a value is known to be non-negative will be profitable (much like GTF_IND_NONNULL).

}

if ((user != nullptr) && user->OperIs(GT_ASG) && (user->AsOp()->gtOp1 == node))
{
lclVarFlags |= GTF_VAR_DEF;
Expand Down