Update the JIT to track Span.Length and ReadOnlySpan.Length as "never negative"
#81055
Conversation
ghost
assigned 
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsThis doesn't do extensive propagation or anything and primarily just focuses on the "normal" case where the imported There is room to potentially track this metadata further through
|
|
The local diff for So nothing "amazing" but there is some nice wins in known hot methods/scenarios. Most of the diffs are changing The diffs are notable "small" for corelib because we have a lot of places where we intentionally do tricks to avoid sign-extension and treat the length as "unsigned" already. I expect a higher band of profit in user code and the general ability to simplify our own logic in some scenarios rather than having the manual casting. |
|
Also apply to Array .Length? |
|
Array.Length is already treated as never negative. |
|
Size savings are basically what I saw locally. There is no perf difference on x64 or x86. Arm64 shows up as "overall +0.00%" with |
|
CC. @dotnet/jit-contrib |
|
Updated to track the info as part of |
| @@ -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); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
tannergooding
force-pushed
the
span-intrin
branch
from
3396758
to
618226e
Compare
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
This resolves #59922
This doesn't do extensive propagation or anything and primarily just focuses on the "normal" case where the imported
GT_FIELDis promoted to aGT_LCL_VAR.There is room to potentially track this metadata further through
FIELD_ADDR,LCL_VAR_ADDR,LCL_FLD, andLCL_FLD_ADDR. There is likewise potential to make this "attribute" driven. For example, it would be possible to effectively create an internalIsNeverNegativeattribute and put it on things likeList<T>.Lengthor similar and make this a bigger feature.It's worth calling out that it is possible for a user to use
unsafecode to modify aSpan<T>/ROSpan<T>'slength field to be negative. However, much like with mutating astatic readonlyfield or using reflection/unsafe to modify other things considered "immutable" we can consider such things as strictly "undefined behavior".Much like with
GT_ARR_LENthis doesn't cover all cases of new locals getting created and it only participates whereIsNeverNegativewas already being used. We have various issues covering codegen improvements related toIntegralRange. Once this is in, future improvements to utilizeIsNeverNegativeinstead of purely relying on base type orGTF_UNSIGNEDwill implicitly light up for all ofT[],Span<T>, andROSpan<T>.