Runtime: [Intrinsic] CompareEqual does not use the memory source instruction

[redknightlois]

I was trying to optimize SequenceCompareTo beyond the point where @benaadams pushed it and stumbled upon various JIT issues controlling the output.

From this code a few are showcased.

Avx2.CompareEqual(LoadVector256(ref first, (IntPtr)lengthAndOffset), LoadVector256(ref second, (IntPtr)lengthAndOffset))

It generates:

movsxd      rdi,esi      
vmovupd     ymm0,ymmword ptr [r11+rdi]  # allocation of usable register should prevent the previous instruction
movsxd      rdi,esi      # useless move
vmovupd     ymm1,ymmword ptr [r10+rdi]  
vpcmpeqb    ymm0,ymm0,ymm1   

I doubt that the movsxd creates a false dependency, but it could use cpu registers which will impact the loop. Probably the worst setback is that is:

vmovupd     ymm1,ymmword ptr [r10+rdi]  
vpcmpeqb    ymm0,ymm0,ymm1   

which could become instead:

vpcmpeqb    ymm0, ymm0, ymmword ptr [r10+rdi]  

using the memory version instead we avoid a data dependency and also using extra physical registers.

Removing the movs and the compare encoding the code should look like:

movsxd      rdi,esi   # outside the loop if really necessary
Loop:
vmovupd     ymm0,ymmword ptr [r11+rdi]
vpcmpeqb    ymm0, ymm0, ymmword ptr [r10+rdi] 
vpmovmskb   edi,ymm0  
...
add       rdi, -32
jna         Loop

category:cq
theme:hardware-intrinsics
skill-level:expert
cost:medium

[benaadams]

The double movsxd could probably be avoided by casting once:

IntPtr offset = (IntPtr)lengthAndOffset;
Avx2.CompareEqual(LoadVector256(ref first, offset), LoadVector256(ref second, offset)

Not sure about the other things though.

[tannergooding]

@redknightlois, if you could get this down to a simple repro and/or provide the JIT dump, it would make it easier to determine why this isn't meeting the IsContainableOp checks in lowering (see https://github.com/dotnet/coreclr/blob/master/src/jit/lowerxarch.cpp#L2450).

Avx2.CompareEqual is a commutative simple SIMD intrinsic that requires the VEX encoding, so it should be getting marked as:

• SupportsAlignedSimdLoads = false
• SupportsUnalignedSimdLoads = true
• SupportsGeneralLoads = true

The second operand should not be a HWIntrinsic, so it should come down to isContainableMemoryOp (https://github.com/dotnet/coreclr/blob/85c61f151c04c1d3cec782532b17969795f07a93/src/jit/lsrabuild.cpp#L683)

[redknightlois]

@tannergooding Probably not simple, but the repro :)
https://gist.github.com/redknightlois/0c2a301006f817e41c364666a422ca63

[redknightlois]

Should be solved by https://github.com/dotnet/coreclr/pull/22944

[CarolEidt]

@redknightlois - can you confirm whether dotnet/coreclr#22944 addressed this?

[redknightlois]

It does not, you can check it on the code generated at the large loop on:
https://github.com/dotnet/coreclr/blob/52aff202cd382c233d903d432da06deffaa21868/src/System.Private.CoreLib/shared/System/SpanHelpers.Byte.cs#L1406

vmovupd     ymm0,ymmword ptr [rcx+r11]  
vmovupd     ymm1,ymmword ptr [r8+r11]  
vpcmpeqb    ymm0,ymm0,ymm1  
vpmovmskb   r10d,ymm0  

Should emit instead:

vmovupd     ymm0,ymmword ptr [rcx+r11]  
vpcmpeqb    ymm0,ymm0,ymmword ptr [r8+r11]  
vpmovmskb   r10d,ymm0  

Checked on: 3.0.0-preview6-27622-10

[mikedn]

Those 2 LoadVector256 calls are likely to lead to a "broken" expression tree, that would require forward substitution or other approach to make it work.

[tannergooding]

It looks to only be a problem around the LoadVector256(ref byte start, IntPtr offset) helper. Pinning and using Avx.LoadVector256(byte* address) correctly folds the second load into the pcmpeqb instruction.

00007FFC4D465EDA  vmovdqu     ymm0,ymmword ptr [rax+rcx]  
00007FFC4D465EDF  vpcmpeqb    ymm0,ymm0,ymmword ptr [rdx+rcx]  
00007FFC4D465EE4  vpmovmskb   r8d,ymm0

[mikedn]

Yeah, basically this happens pretty much every time you have something like call(call1, call2, ... callN), in theory it could be imported as a single expression tree but the JIT has issues with getting the order right and insists in splitting the expression into multiple trees. In this case:

***** BB01, stmt 1
     (  6,  5) [000016] ------------              *  STMT      void  (IL   ???...  ???)
N006 (  6,  5) [000047] -A-XG---R---              \--*  ASG       simd32 (copy) $VN.Void
N005 (  1,  1) [000046] D------N----                 +--*  LCL_VAR   simd32 V04 tmp1         d:1
N004 (  6,  5) [000044] U--XG-------                 \--*  IND       simd32 <l:$241, c:$240>
N003 (  3,  3) [000041] ------------                    \--*  ADD       byref  $140
N001 (  1,  1) [000000] ------------                       +--*  LCL_VAR   byref  V00 arg0         u:1 (last use) $80
N002 (  1,  1) [000001] ------------                       \--*  LCL_VAR   long   V02 arg2         u:1 $c0

***** BB01, stmt 2
     (  6,  5) [000022] ------------              *  STMT      void  (IL   ???...  ???)
N006 (  6,  5) [000068] -A-XG---R---              \--*  ASG       simd32 (copy) $VN.Void
N005 (  1,  1) [000067] D------N----                 +--*  LCL_VAR   simd32 V05 tmp2         d:1
N004 (  6,  5) [000065] U--XG-------                 \--*  IND       simd32 <l:$243, c:$242>
N003 (  3,  3) [000062] ------------                    \--*  ADD       byref  $141
N001 (  1,  1) [000007] ------------                       +--*  LCL_VAR   byref  V01 arg1         u:1 (last use) $81
N002 (  1,  1) [000008] ------------                       \--*  LCL_VAR   long   V02 arg2         u:1 (last use) $c0

***** BB01, stmt 3
     (  5,  5) [000027] ------------              *  STMT      void  (IL   ???...  ???)
N005 (  5,  5) [000026] ------------              \--*  RETURN    int    $2c0
N004 (  4,  4) [000025] ------------                 \--*  HWIntrinsic int    ubyte MoveMask $281
N003 (  3,  3) [000024] ------------                    \--*  HWIntrinsic simd32 ubyte CompareEqual $280
N001 (  1,  1) [000017] ------------                       +--*  LCL_VAR   simd32 V04 tmp1         u:1 (last use) $1c2
N002 (  1,  1) [000023] ------------                       \--*  LCL_VAR   simd32 V05 tmp2         u:1 (last use) $1c5

I'll see if I can tweak my forward substitution experiment to handle this case (but obviously even if I do that it will be too late for 3.0). Though it would be better if the JIT stops splitting trees like that. It just results in more variables and more work for the downstream phases.