Fix Unnecessary sign-extension for (nuint)span.Length

[En3Tho]

Fixes #84564

After review upd:
This adds a new CORINFO_FLG_SPAN flag and a IsSpan property to simplify handling of unpromoted span.Length loads and possibly other optimizations of Spans

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Test fix for #84564

Unconditionally set isNeverNegative in morph, use that data when optimizing casts

Author:	En3Tho
Assignees:	-
Labels:	area-CodeGen-coreclr, community-contribution
Milestone:	-

[jakobbotsch]

I think the right way to do this is to add CORINFO_FLG_SPAN, have the EE return it from getClassAttribs and then mark Span<T> locals from inside lvSetStruct (or perhaps just as part of promotion). Then we can remove the old lvIsNeverNegative in favor of checking whether a local is a promoted length field (or for your case whether it is the right indirection off of a span local). This PR marks a Span<T> local as "never negative" which doesn't really make much sense.

FWIW, it's only a matter of time before we are going to end up with CORINFO_FLG_SPAN one way or the other -- I expect we will need it for #82946 too.

Edit: For this PR I would be fine with adding an lvIsSpan next to lvIsNeverNegative and then just setting that where you are setting lvIsNeverNegative instead.

[En3Tho]

Thank you! If adding CORINFO_FLG_SPAN is preferred then I can look into that instead. I've decided to allocate more time to architectures/compilers so RyuJit is kinda obvious option for me. This issue looked "easy" but w.o context it's obviously hard to know where things are going and what change is actually worthy and what is just useless/harmful.

Otherwise should I set lvIsSpan for both varDsc and fieldVarDsc like I do with lvIsNeverNegative now?

[jakobbotsch]

If adding CORINFO_FLG_SPAN is preferred then I can look into that instead. I've decided to allocate more time to architectures/compilers so RyuJit is kinda obvious option for me. This issue looked "easy" but w.o context it's obviously hard to know where things are going and what change is actually worthy and what is just useless/harmful.

Awesome! Looking forward to more PRs!

Otherwise should I set lvIsSpan for both varDsc and fieldVarDsc like I do with lvIsNeverNegative now?

varDsc should be set as lvIsSpan and fieldVarDsc should be set as lvIsNeverNegative (when isSpanLength) is true.

[En3Tho]

Adding GT_LCL_FLD did not affect arm. On linux x64 the field is already promoted it seems. So for now keep this strictly for win x64. I will look more closely at arm.

[jakobbotsch]

Adding GT_LCL_FLD did not affect arm. On linux x64 the field is already promoted it seems. So for now keep this strictly for win x64. I will look more closely at arm.

I'd add the GT_LCL_FLD handling, if only for consistency. It should be a quite simple check to add.

[En3Tho]

I've applied you suggestion and ran jit-format on it too.
Also, I've added GT_LCL_FLD handling which only includes IsNeverNegative check. From what I've understood lvNormalizeOnStore applies only to GT_LCL_VAR?

[jakobbotsch]

Looks fine to me, just a few nits. The diffs are small, but I think taking this change anyway is fine since adding LclVarDsc::IsSpan seems good and we'll likely need it for #82946 too.

[jakobbotsch]

@MihuBot

[jakobbotsch]

The diffs are small

It's hard to say actually, since this causes so many contexts to miss. Let's see with jit-diff.

[En3Tho]

@jakobbotsch Is it okay that we now have both lvIsNeverNegative and lvIsSpan fields in LclVarDsc? I remember the plan was to remove lvIsNeverNegative to keep the type from growing but I don't see how it can be removed because it seems quite generic unlike lvIsSpan

Also, jit-diffs are zero? This is weird.

[jakobbotsch]

@jakobbotsch Is it okay that we now have both lvIsNeverNegative and lvIsSpan fields in LclVarDsc? I remember the plan was to remove lvIsNeverNegative to keep the type from growing but I don't see how it can be removed because it seems quite generic unlike lvIsSpan

I think it's fine, we can leave it for now.

Also, jit-diffs are zero? This is weird.

@MihaZupan's bot runs on linux-x64 where spans are passed by value. It means we always promote them so your optimization does not kick in.

[En3Tho]

https://dev.azure.com/dnceng-public/public/_build/results?buildId=422571&view=ms.vss-build-web.run-extensions-tab

Small positive diffs - win86/64 benefits the most and even a small win for non-win systems in System.Security.Cryptography.SP800108HmacCounterKdfImplementationManaged:DeriveBytesOneShot(System.ReadOnlySpan1` method

Overall TP hit is +0.01-0.02%

Most notable win is for Win-x64 in System.Text.RegularExpressions.Generated.<RegexGenerator_g>F74B1AE921BCEFE4BA601AA541C7A23B1CA9711EA81E8FE504B5B6446748E035A__CreateConnectionStringValidKeyRegex_0+RunnerFactory+Runner:TryMatchAtCurrentPosition(System.ReadOnlySpan1[ushort]):ubyte:this (FullOpts)` - 14 bytes

-       mov      eax, ecx
-       mov      edx, dword ptr [rbx+0x08]
-       movsxd   r8, edx
-       cmp      rax, r8
-       jge      SHORT G_M14194_IG15
-       cmp      ecx, edx
-       jae      G_M14194_IG20
+       mov      eax, dword ptr [rbx+0x08]
+       cmp      ecx, eax
+       jae      SHORT G_M14194_IG15

[En3Tho]

Removed some code changes in lclmorph (basically restored the original state). Let's see if my changes were redundant.

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

The SPMI collection I did yesterday failed. I'm still trying to make a collection based on this PR. Currently the diffs are misleading due to the misses.

[En3Tho]

New spmi diff result is even weirder than no diffs at all :D

https://dev.azure.com/dnceng-public/public/_build/results?buildId=425438&view=ms.vss-build-web.run-extensions-tab

[jakobbotsch]

New spmi diff result is even weirder than no diffs at all :D

https://dev.azure.com/dnceng-public/public/_build/results?buildId=425438&view=ms.vss-build-web.run-extensions-tab

Those diffs look like they are correct, some of the collection from yesterday succeeded and those diffs are in the collections with low amount of misses.
Though most of these diffs look to be from the JIT being able to fold out debug asserts (SPMI uses checked libraries, it's on our TODO to use release libraries). Specifically this assert can frequently be folded away when there's an implicit ReadOnlySpan conversion:

runtime/src/libraries/System.Private.CoreLib/src/System/Span.cs

Line 133 in 7abea9e

Debug.Assert(length >= 0);

The implicit conversion from ReadOnlySpan accesses the _length field directly, so it does not go through the existing intrinsic importation path in the JIT, and thus does not get the non-negative annotation. With the new approach (and doing it from promotion) it does.

Sadly I'm still hitting issues getting a full clean SPMI collection... I don't think we can merge this PR until we get that since otherwise we will have too many misses.

[En3Tho]

It's no problem. There is plenty of time till .Net 9 :D. I will try find another small thing to work on meanwhile. Thank you for your help again!

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

Thank you!