Jit doesn't recognise ldstr as non-null

[benaadams]

public ReadOnlySpan<char> Hello() => "Hello".AsSpan();

.method public hidebysig 
    instance valuetype [System.Memory]System.ReadOnlySpan`1<char> Hello () cil managed 
{
    // Method begins at RVA 0x2050
    // Code size 11 (0xb)
    .maxstack 8

    IL_0000: ldstr "Hello"
    IL_0005: call valuetype [System.Memory]System.ReadOnlySpan`1<char> [System.Memory]System.MemoryExtensions::AsSpan(string)
    IL_000a: ret
}

G_M23320_IG01:
       nop      

G_M23320_IG02:
       mov      rax, 0xD1FFAB1E
       mov      rax, gword ptr [rax]
       test     rax, rax               ; not needed
       jne      SHORT G_M23320_IG03    ; not needed
       xor      rcx, rcx               ; not needed
       xor      r8d, r8d               ; not needed
       jmp      SHORT G_M23320_IG04    ; not needed

G_M23320_IG03:
       mov      ecx, dword ptr [rax]   ; not needed   
       mov      rcx, rax               ; not needed             ?
       cmp      dword ptr [rcx], ecx   ; not needed
       add      rcx, 12                ; r8d, dword ptr [rax+8] ?
       mov      r8d, dword ptr [rax+8] ; add      rax, 12       ?

G_M23320_IG04:
       mov      bword ptr [rdx], rcx   ; bword ptr [rdx], rax   ?
       mov      dword ptr [rdx+8], r8d
       mov      rax, rdx

G_M23320_IG05:
       ret      

; Total bytes of code 56, prolog size 5 for method Program:Hello():struct:this

/cc @GrabYourPitchforks

/cc @KrzysztofCwalina as you mentioned in dotnet/roslyn#24621 (comment)

Sure, but it has nothing to do with ranges. We could do the same optimization for

ReadOnlySpan<char> x = "str"; 

category:cq
theme:basic-cq
skill-level:expert
cost:large

[GrabYourPitchforks]

I think this can be even further optimized - the JIT should know the length of the string being loaded so can burn it in to the code gen. There's no need to query the string length and move it to the appropriate location.

Sample:

mov rax, 123456h  ; whatever the known entry index is that corresponds to this string
mov rax, qword ptr [rax]  ; rax now contains the string object reference itself
mov rax, qword ptr [rax + 8h]  ; rax now contains a reference to the beginning of the string data
mov qword ptr [rdx], rax  ; [rdx] = Span.Pointer
mov dword ptr [rdx + 8h], 5  ; [rdx + 8h] = Span.Length, known length of fixed string "Hello"

[mikedn]

Eh, gtFoldExpr shoots itself in the foot - it thinks that GT_CNS_STR is a constant but then gtFoldExprConst fails to handle GT_CNS_STR ==/!= null. With that out of the way we get:

G_M55889_IG01:
       0F1F440000           nop
G_M55889_IG02:
       48B85033C1EA24020000 mov      rax, 0x224EAC13350
       488B00               mov      rax, gword ptr [rax]
       8B10                 mov      edx, dword ptr [rax]
       488BD0               mov      rdx, rax
       3912                 cmp      dword ptr [rdx], edx
       4883C20C             add      rdx, 12
       8B4008               mov      eax, dword ptr [rax+8]
       488911               mov      bword ptr [rcx], rdx
       894108               mov      dword ptr [rcx+8], eax
       488BC1               mov      rax, rcx
G_M55889_IG03:
       C3                   ret

Still need to get rid of a NULLCHECK and propagate the constant string length.

[benaadams]

From a register point of view would swapping the arguments to the span .ctor help any?

ReadOnlySpan(ref T ptr, int length)

to

ReadOnlySpan(int length, ref T ptr)

Since the layout of strings and arrays is [length][first element]

[mikedn]

From a register point of view would swapping the arguments to the span .ctor help any?

In the constant case I don't see why it would matter.

propagate the constant string length.

That seems to be unexpectedly complicated, the VM keeps the JIT in the dark about the string length. The JIT interface will probably need to be extended.

[AndyAyersMS]

I've run across the idea of allowing the jit to know the length and even the contents of ldstrs now and then. As @mikedn says at jit time (or prejit time) the string may not yet be materialized as a string object, so new queries back into the VM are needed.

Once the string is materialized the jit can also inspect it, so we can do similar things with readonly static strings.

Can we tie this to some motivating cases in Kestrel?

[mikedn]

With (both) nullchecks removed:

       48B85033766563020000 mov      rax, 0x26365763350
       488B00               mov      rax, gword ptr [rax]
       488D500C             lea      rdx, bword ptr [rax+12]
       8B4008               mov      eax, dword ptr [rax+8]
       488911               mov      bword ptr [rcx], rdx
       894108               mov      dword ptr [rcx+8], eax
       488BC1               mov      rax, rcx

With some horrible hack to get the string length:

       48B85033E92E1A020000 mov      rax, 0x21A2EE93350
       488B00               mov      rax, gword ptr [rax]
       4883C00C             add      rax, 12
       BA09000000           mov      edx, 9
       488901               mov      bword ptr [rcx], rax
       895108               mov      dword ptr [rcx+8], edx
       488BC1               mov      rax, rcx

Interestingly, this shows a problem with constant propagation.

So, null checks are pretty easy to get rid of during import/inline/morph. Might be interesting/useful to also support null check removal in VN but it's not clear how to do that because by the time we reach VN GT_CNS_STR nodes are gone, replaced by indirections and handles.

Dealing with constant lengths, well, that's another story.

[am11]

Once the string is materialized the jit can also inspect it, so we can do similar things with readonly static strings.

Do these static readonly after materialization get classified as GT_CNS_STR, where values are purely runtime constants; i.e. maybe skip cases like:

static readonly string sm_str = "default";
static X()
{
    if (someRuntimeCondition())
    {
        sm_str = "overwritten";
    }
}

(i was thinking if static readonly string could take advantage of @EgorBo's optimization for const strings: #1378)

[EgorBo]

@am11 #42087

[EgorBo]

Can be closed I guess (via #1378 + #37245)

static ReadOnlySpan<char> ToSpan() => "literal";

; Method ToSpan():System.ReadOnlySpan`1[Char]
G_M54852_IG01:
						;; bbWeight=1    PerfScore 0.00

G_M54852_IG02:
       mov      rax, 0xD1FFAB1E
       mov      rax, gword ptr [rax]
       add      rax, 12
       mov      bword ptr [rcx], rax
       mov      dword ptr [rcx+8], 7
       mov      rax, rcx
						;; bbWeight=1    PerfScore 4.75

G_M54852_IG03:
       ret      
						;; bbWeight=1    PerfScore 1.00
; Total bytes of code: 31