JIT: optFoldNullCheck missed cases

[AndyAyersMS]

optFoldNullCheck's pattern match misses some cases.

See dotnet/coreclr#23850 (comment) for one such example (though perhaps that could also be addressed by better handling of commas in add trees).

Will add others as I run across them.

category:cq
theme:basic-cq
skill-level:intermediate
cost:small

[erozenfeld]

cc @dotnet/jit-contrib

[AndyAyersMS]

Another diff from dotnet/coreclr#23850

; System.Private.Xml -- XmlSqlBinaryReader:AddName():this
G_M9553_IG02:
       mov      rcx, rsi
       call     XmlSqlBinaryReader:ParseText():ref:this
       mov      rdi, rax
       cmp      dword ptr [rsi], esi
       lea      rdx, bword ptr [rsi+224]
       add      rdx, 16
       mov      ebx, dword ptr [rdx]

Some pre-existing cases

; System.Private.Xml -- SR:Format(ref,ref,ref):ref
G_M51756_IG03:
       cmp      dword ptr [rdx], edx
       lea      rcx, bword ptr [rdx+12]
       mov      bword ptr [rsp+48H], rcx
       mov      rcx, bword ptr [rsp+48H]
       mov      edx, dword ptr [rdx+8]
       mov      r8, rbx
       call     String:JoinCore(long,int,ref):ref

; System.Private.Xml -- XmlSqlBinaryReader:NameFlush():this
G_M54165_IG02:
       cmp      dword ptr [rsi], esi
       lea      r8, bword ptr [rsi+224]
       mov      dword ptr [rsi+244], 1

; System.Private.Xml -- XmlSqlBinaryReader:ImplReadPI():this

G_M28197_IG02:
       cmp      dword ptr [rsi], esi
       lea      rcx, bword ptr [rsi+248]
       mov      rdi, gword ptr [rsi+224]

; System.Private.XML -- XmlSqlBinaryReader:ValueAsString(int):ref:this
G_M38778_IG20:
       mov      edx, dword ptr [rax]
       mov      rsi, rax
       cmp      dword ptr [rsi], esi

Example in the spirit of #12436

; System.Memory -- BuffersExtensions:PositionOfMultiSegment(byref,short):struct
G_M44022_IG18:
   ... 
       cmp      dword ptr [rax], eax
       lea      rcx, bword ptr [rax+8]
       mov      r8, qword ptr [rcx-8]

; System.Private.CoreLib -- String:Ctor(long):ref:this
; probably tied up with pinning

G_M52690_IG06:
       mov      ecx, edi
       call     String:FastAllocateString(int):ref
       mov      rbx, rax
       cmp      dword ptr [rbx], ebx
       lea      r8, bword ptr [rbx+12]
       mov      bword ptr [rsp+28H], r8

[ArtBlnd]

I'll take a look close to it after solving range check issues.

[ArtBlnd]

Can you explain what actual case of C# codes are missed in case? as simplified code.

[erozenfeld]

Copying the example from dotnet/coreclr#23850 (comment) here

; System.Transactions.Local   InternalEnlistment:FinishEnlistment():this
        mov      rax, gword ptr [rcx+32]
-       add      rax, 260
+       cmp      dword ptr [rax], eax
+       add      rax, 232
+       add      rax, 28
        inc      dword ptr [rax]

[ArtBlnd]

Can I have it as Jit-Dump please? Thanks :)

[erozenfeld]

I'm working on this and have null checks removed in several of the examples above.
@AndyAyersMS I don't understand one of your examples. I don't think the null check should be removed here. Am I missing something?

I added several instructions to what you have above.

; System.Private.CoreLib -- String:Ctor(long):ref:this
; probably tied up with pinning

G_M52690_IG06:
       mov      ecx, edi
       call     String:FastAllocateString(int):ref
       mov      rbx, rax
       cmp      dword ptr [rbx], ebx
       lea      r8, bword ptr [rbx+12]
       mov      bword ptr [rsp+28H], r8
       mov      rcx, bword ptr [rsp+28H]
       shl      edi, 1
       mov      r8d, edi
       mov      rdx, rsi
       call     Buffer:Memmove(long,long,long)

[erozenfeld]

#1735 eliminates null checks in the following cases above:

; System.Private.Xml -- XmlSqlBinaryReader:AddName():this
; System.Private.Xml -- SR:Format(ref,ref,ref):ref
; System.Private.Xml -- XmlSqlBinaryReader:NameFlush():this
; System.Private.Xml -- XmlSqlBinaryReader:ImplReadPI():this
; System.Transactions.Local InternalEnlistment:FinishEnlistment():this

The current version of
; System.Memory -- BuffersExtensions:PositionOfMultiSegment(byref,short):struct
doesn't have the pattern quoted above. #1735 does eliminate a nullcheck in that method:

-cmp      dword ptr [rax], eax
mov      rcx, qword ptr [rax]

; System.Private.CoreLib -- String:Ctor(long):ref:this
is an invalid example (see my comment above https://github.com/dotnet/coreclr/issues/23903#issuecomment-567763714)

; System.Private.XML -- XmlSqlBinaryReader:ValueAsString(int):ref:this
has the following pattern:
comma(tmp1 = ind(const1), nullcheck(tmp1))
followed by
arr_length(ind(const1))
This is beyond the scope of optFoldNullCheck.

[erozenfeld]

Fixed by #1735