JIT: Strange redunant 'cmp' in Unsafe.As<TFrom, TTo>() codegen

[hypeartist]

Spotted in .NET 6.0.* and .NET 7.0.100-preview.2.22078.1:

Sample code:

public unsafe struct Test
{	
	private int[] A;
	
	public int Sum_GetArrayDataReferenceCalled() 
	{
		var s = 0;
		var l = (nuint)A.Length;
		ref var ar = ref MemoryMarshal.GetArrayDataReference(A);
		for(nuint i = 0; i < l; i++) 
		{
			s += Unsafe.Add(ref ar, i);
		}
		return s;
	}
	
	// This version is just the same as above but has a manually inlined 'MemoryMarshal.GetArrayDataReference' 
	// allowing us to narrow down (I believe) the source of the issue
	public int Sum_GetArrayDataReferenceInlined() 
	{
		var s = 0;
		var l = (nuint)A.Length;
		var a = A;
		ref var d = ref Unsafe.As<int[], RawArrayData>(ref a); //  <---- ???
		ref var ar = ref Unsafe.As<byte, int>(ref d.Data);
		
		for(nuint i = 0; i < l; i++) 
		{
			s += Unsafe.Add(ref ar, i);
		}
		return s;
	}
	
	private sealed class RawArrayData
	{
		public uint Length;
		public uint Padding;
		public byte Data;
	}
}

Result codegen:

; ================================================================================
; Test.Sum_GetArrayDataReferenceCalled()
; 35 (0x23) bytes
; 13 (0xD) instructions

;       var s = 0;
        xor     eax,eax

;       var l = (nuint)A.Length;
        mov     rdx,[rcx]
        mov     ecx,[rdx+8]

;       ref var ar = ref MemoryMarshal.GetArrayDataReference(A);
        cmp     [rdx],edx
        add     rdx,10h

;       for(nuint i = 0; i < l; i++) 
;           ^^^^^^^^^^^
        xor     r8d,r8d

;       for(nuint i = 0; i < l; i++) 
;                        ^^^^^
        test    rcx,rcx
        jbe     LBL_1

LBL_0:
;           s += Unsafe.Add(ref ar, i);
        add     eax,[rdx+r8*4]

;       for(nuint i = 0; i < l; i++) 
;                               ^^^
        inc     r8

;       for(nuint i = 0; i < l; i++) 
;                        ^^^^^
        cmp     r8,rcx
        jb      LBL_0

LBL_1:
;       return s;
        ret

; ================================================================================
; Test.Sum_GetArrayDataReferenceInlined()
; 35 (0x23) bytes
; 13 (0xD) instructions

;       var s = 0;
        xor     eax,eax

;       var l = (nuint)A.Length;
        mov     rdx,[rcx]
        mov     ecx,[rdx+8]

;       ref var d = ref Unsafe.As<int[], RawArrayData>(ref a); //  <---- ???
;       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        cmp     [rdx],edx
        add     rdx,10h

;       for(nuint i = 0; i < l; i++) 
;           ^^^^^^^^^^^
        xor     r8d,r8d

;       for(nuint i = 0; i < l; i++) 
;                        ^^^^^
        test    rcx,rcx
        jbe     LBL_1

LBL_0:
;           s += Unsafe.Add(ref ar, i);
        add     eax,[rdx+r8*4]

;       for(nuint i = 0; i < l; i++) 
;                               ^^^
        inc     r8

;       for(nuint i = 0; i < l; i++) 
;                        ^^^^^
        cmp     r8,rcx
        jb      LBL_0

LBL_1:
;       return s;
        ret

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgBkBLAOwEcBuGvSbkUA2o2YsASgFcuGDvhgsAwhHwAHDgBsYUAMp6AbhzAxcYoZNnzFygJLy9EDYagmzFwRNYA5GUpQpl7iwtJyCkosjhhBXLjBLAAaABxIlj7htlExcQlgIYIAzAxyuNgAZjAMuLEyYBgMACrmGDQA3gCQNJ0aQUbYGNXcGADaALoMAIJi3dSdxCUjDPoBAPoA4jAYU1BQ2ACeACKD2FIwVbBcZirY2roAJgAUAJQMPe09nQNQNQwAvAw6LNvthftoAQwnlwZCMXlN2DAuABzDAACxBsAqDB+DDBkKxDAAsjB8NADkSwbg0XcWFsdntDicMGcLnokWYnlMXiCKtBobD5AwOJDgcKGAAeBjaPjCgDUcreX0+806nVwDDlgIAqvFKsopg9noSwWhhTyvgBfL7EADsNVm1vmPQA9C7mmiOBqjHoEhAuMKNQArGS1Bjo6rlJR4jXYYAQH0MYAyRo02MMfDYGF3bQHYVcbTcGAPBgAchJZKgFKpNO0dO2u32x1O50uHJgpfe8zdePuEAA7txkaUNRgIAwuGCoAOGA8BwGnvYkzBCzAfW8IzUIDIoGYGBBsZuvbgZDAeot841VvhNg3Gc2Wa32dcYI5C1xi68u50VWrcRrAWBL5cQhQEBThBE2CRVEMWA/FsEhGYvkJXES0BQldXKKoWCmXAJRGCYzSkbB+0bJlTgAPieE0eQYHtJTgRi4AYAB+NjkIuHF4N+dDOMw/UcLw4ADiGM0RiowkHhYZlsAtVUvj5KBwKFEVANlEUpRleVFW/H8vnVTUdT1bDDWNTjTXNEEnTVO0HR6ayej6DgBiGGoYDuYsmFIBhiNI+8ZI+G0SkFRooJRdEQQvEKGAABWwI0h0ikphNcmTHRoS0gA

category:cq
theme:codegen
skill-level:beginner
cost:small
impact:medium

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

Issue Details

---

Spotted in .NET 6.0.* and .NET 7.0.100-preview.2.22078.1:

Sample code:

public unsafe struct Test
{	
	private int[] A;
	
	public int Sum_GetArrayDataReferenceCalled() 
	{
		var s = 0;
		var l = (nuint)A.Length;
		ref var ar = ref MemoryMarshal.GetArrayDataReference(A);
		for(nuint i = 0; i < l; i++) 
		{
			s += Unsafe.Add(ref ar, i);
		}
		return s;
	}
	
	// This version is just the same as above but has a manually inlined 'MemoryMarshal.GetArrayDataReference' 
	// allowing us to narrow down (I believe) the source of the issue
	public int Sum_GetArrayDataReferenceInlined() 
	{
		var s = 0;
		var l = (nuint)A.Length;
		var a = A;
		ref var d = ref Unsafe.As<int[], RawArrayData>(ref a); //  <---- ???
		ref var ar = ref Unsafe.As<byte, int>(ref d.Data);
		
		for(nuint i = 0; i < l; i++) 
		{
			s += Unsafe.Add(ref ar, i);
		}
		return s;
	}
	
	private sealed class RawArrayData
	{
		public uint Length;
		public uint Padding;
		public byte Data;
	}
}

Result codegen:

; ================================================================================
; Test.Sum_GetArrayDataReferenceCalled()
; 35 (0x23) bytes
; 13 (0xD) instructions

;       var s = 0;
        xor     eax,eax

;       var l = (nuint)A.Length;
        mov     rdx,[rcx]
        mov     ecx,[rdx+8]

;       ref var ar = ref MemoryMarshal.GetArrayDataReference(A);
        cmp     [rdx],edx
        add     rdx,10h

;       for(nuint i = 0; i < l; i++) 
;           ^^^^^^^^^^^
        xor     r8d,r8d

;       for(nuint i = 0; i < l; i++) 
;                        ^^^^^
        test    rcx,rcx
        jbe     LBL_1

LBL_0:
;           s += Unsafe.Add(ref ar, i);
        add     eax,[rdx+r8*4]

;       for(nuint i = 0; i < l; i++) 
;                               ^^^
        inc     r8

;       for(nuint i = 0; i < l; i++) 
;                        ^^^^^
        cmp     r8,rcx
        jb      LBL_0

LBL_1:
;       return s;
        ret

; ================================================================================
; Test.Sum_GetArrayDataReferenceInlined()
; 35 (0x23) bytes
; 13 (0xD) instructions

;       var s = 0;
        xor     eax,eax

;       var l = (nuint)A.Length;
        mov     rdx,[rcx]
        mov     ecx,[rdx+8]

;       ref var d = ref Unsafe.As<int[], RawArrayData>(ref a); //  <---- ???
;       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        cmp     [rdx],edx
        add     rdx,10h

;       for(nuint i = 0; i < l; i++) 
;           ^^^^^^^^^^^
        xor     r8d,r8d

;       for(nuint i = 0; i < l; i++) 
;                        ^^^^^
        test    rcx,rcx
        jbe     LBL_1

LBL_0:
;           s += Unsafe.Add(ref ar, i);
        add     eax,[rdx+r8*4]

;       for(nuint i = 0; i < l; i++) 
;                               ^^^
        inc     r8

;       for(nuint i = 0; i < l; i++) 
;                        ^^^^^
        cmp     r8,rcx
        jb      LBL_0

LBL_1:
;       return s;
        ret

Author:	hypeartist
Assignees:	-
Labels:	area-CodeGen-coreclr, untriaged
Milestone:	-

[SingleAccretion]

This is an interesting issue. A bit smaller reproduction:

[MethodImpl(MethodImplOptions.NoInlining)]
private ref int Problem(ClassWithFields a)
{
    _ = a.ObjWithField.First;

    return ref a.ObjWithField.First;
}

sealed class ClassWithFields
{
    public int First;
    public ClassWithFields ObjWithField;
}

First, we start in morph, where we have to add a null check for a.ObjWithField, because morph does not track assertions for indirections (and thus does not know a.ObjWithField cannot not null).

Fortunately, VN does work with indirections, and gives both a.ObjWithField accesses the same liberal VN, but different conservative VNs. This is important because the null check removal happens in assertion propagation that uses conservative VNs. Fortunately for us, CSE is able to turn liberal VNs into conservative by replacing the indirections with locals, which it will do in this case, but then it doesn't help because for the null check morphing we inserted a temporary that will still have its "detached" conservative VN that assertion propagation will not know it is actually the same value as the one that was CSEd:

***** BB01
STMT00000 ( 0x000[E-] ... 0x00B )
N013 (  8,  8) [000004] -A-XG-------              *  COMMA     void   <l:$301, c:$300>
N011 (  8,  8) [000002] -A-XG-------              +--*  IND       int    <l:$244, c:$243>
N010 (  6,  6) [000011] -A-XG--N----              |  \--*  ADD       byref  <l:$144, c:$143>
N008 (  5,  5) [000030] -A-XG-------              |     +--*  COMMA     ref    <l:$203, c:$202>
N006 (  4,  4) [000028] -A-XG---R---              |     |  +--*  ASG       ref    $VN.Void
N005 (  1,  1) [000027] D------N----              |     |  |  +--*  LCL_VAR   ref    V04 cse0         d:1 <l:$203, c:$202>
N004 (  4,  4) [000001] ---XG-------              |     |  |  \--*  IND       ref    <l:$203, c:$202>
N003 (  2,  2) [000013] -------N----              |     |  |     \--*  ADD       byref  $140
N001 (  1,  1) [000000] ------------              |     |  |        +--*  LCL_VAR   ref    V01 arg1         u:1 $80
N002 (  1,  1) [000012] ------------              |     |  |        \--*  CNS_INT   long   16 field offset Fseq[ObjWithField] $100
N007 (  1,  1) [000029] ------------              |     |  \--*  LCL_VAR   ref    V04 cse0         u:1 <l:$203, c:$202>
N009 (  1,  1) [000010] ------------              |     \--*  CNS_INT   long   32 field offset Fseq[First] $101
N012 (  0,  0) [000003] ------------              \--*  NOP       void   $2c0

***** BB01
STMT00001 ( 0x00C[E-] ... 0x017 )
N011 (  7,  9) [000009] -A-XG-------              *  RETURN    byref  <l:$210, c:$20e>
N010 (  6,  8) [000022] -A-XG--N----              \--*  COMMA     byref  <l:$147, c:$146>
N006 (  3,  5) [000025] -A-XG-------                 +--*  COMMA     void   <l:$210, c:$20e>
N003 (  1,  3) [000015] -A--G---R---                 |  +--*  ASG       ref    <l:$203, c:$208>
N002 (  1,  1) [000014] D------N----                 |  |  +--*  LCL_VAR   ref    V03 tmp1         d:1 <l:$200, c:$85>
N001 (  1,  1) [000031] ------------                 |  |  \--*  LCL_VAR   ref    V04 cse0         u:1 <l:$200, c:$81>
N005 (  2,  2) [000017] ---X---N----                 |  \--*  NULLCHECK byte   <l:$20c, c:$20b>
N004 (  1,  1) [000016] ------------                 |     \--*  LCL_VAR   ref    V03 tmp1         u:1 <l:$200, c:$85>
N009 (  3,  3) [000021] ------------                 \--*  ADD       byref  <l:$142, c:$145>
N007 (  1,  1) [000019] ------------                    +--*  LCL_VAR   ref    V03 tmp1         u:1 (last use) <l:$200, c:$85>
N008 (  1,  1) [000020] ------------                    \--*  CNS_INT   long   32 field offset Fseq[First] $101

-------------------------------------------------------------------------------------------------------------------
GenTreeNode creates assertion:
N004 (  4,  4) [000001] ---XG-------              *  IND       ref    <l:$203, c:$202>
In BB01 New Global Constant Assertion: ($80,$0) V01.01 != null, index = #01
GenTreeNode creates assertion:
N005 (  2,  2) [000017] ---X---N----              *  NULLCHECK byte   <l:$20c, c:$20b>
In BB01 New Global Constant Assertion: ($85,$0) V03.01 != null, index = #02

Ultimately, the issue is that CSE "improves" the transient closure of conservative VNs for the expressions it replaces, and we do not take advantage of that (except in some specific cases with array lengths and bounds checks). It is not clear to me if there's a trivial fix for this.

[JulieLeeMSFT]

cc @jakobbotsch for VN and CSE related issue.
Putting this to future.

[deeprobin]

Was this fixed due intrinsification of Unsafe.As (see #68739)?