Multiple/unnecessary movzx/movsxd with char -> byte -> IntPtr casts

[Sergio0694]

Description

I found a couple places where the JIT seems to be adding unnecessary movzx/movsxd instructions when doing unchecked conversions from char to byte and then up again to IntPtr. It's basically just doing the same operation again.

Consider this simple example (sharplab.io sample here):

public static unsafe IntPtr Convert(char c)
{
    return unchecked((IntPtr)(void*)(byte)c);
}

I get the following:

;  V00 arg0         [V00,T00] (  3,  3   )  ushort  ->  rcx        
;# V01 OutArgs      [V01    ] (  1,  1   )  lclBlk ( 0) [rsp+0x00]   "OutgoingArgSpace"
;* V02 tmp1         [V02    ] (  0,  0   )    long  ->  zero-ref    "Inlining Arg"
;  V03 tmp2         [V03,T01] (  2,  4   )    long  ->  rax         "NewObj constructor temp"
;* V04 tmp3         [V04    ] (  0,  0   )    long  ->  zero-ref    "Inlining Arg"
;
; Lcl frame size = 0

G_M51760_IG01:
						;; bbWeight=1    PerfScore 0.00
G_M51760_IG02:
       0FB7C1               movzx    rax, cx
       0FB6C0               movzx    rax, al
						;; bbWeight=1    PerfScore 0.50
G_M51760_IG03:
       C3                   ret      
						;; bbWeight=1    PerfScore 1.00

; Total bytes of code 7, prolog size 0, PerfScore 2.20

That second movzx rax, al is not actually neeced, right?

Or, this other related case (sharplab.io sample here):

// Assume this has 255 items in it
private static ReadOnlySpan<byte> SomeTable => new byte[] { 1, 2, 3 };

[MethodImpl(MethodImplOptions.AggressiveOptimization)]
public static byte Lookup(char c)
{
    ref byte r0 = ref MemoryMarshal.GetReference(SomeTable);

    return Unsafe.Add(ref r0, unchecked((byte)c));
}

Gives the following:

;  V00 arg0         [V00,T00] (  3,  3   )  ushort  ->  rcx        
;# V01 OutArgs      [V01    ] (  1,  1   )  lclBlk ( 0) [rsp+0x00]   "OutgoingArgSpace"
;* V02 tmp1         [V02    ] (  0,  0   )  struct (16) zero-ref    "struct address for call/obj"
;* V03 tmp2         [V03    ] (  0,  0   )  struct (16) zero-ref    "NewObj constructor temp"
;* V04 tmp3         [V04    ] (  0,  0   )  struct ( 8) zero-ref    "NewObj constructor temp"
;* V05 tmp4         [V05    ] (  0,  0   )  struct (16) zero-ref    ld-addr-op "Inlining Arg"
;* V06 tmp5         [V06    ] (  0,  0   )   byref  ->  zero-ref    "Inlining Arg"
;* V07 tmp6         [V07    ] (  0,  0   )     int  ->  zero-ref    "Inlining Arg"
;* V08 tmp7         [V08    ] (  0,  0   )   byref  ->  zero-ref    V02._pointer(offs=0x00) P-INDEP "field V02._pointer (fldOffset=0x0)"
;* V09 tmp8         [V09    ] (  0,  0   )     int  ->  zero-ref    V02._length(offs=0x08) P-INDEP "field V02._length (fldOffset=0x8)"
;* V10 tmp9         [V10    ] (  0,  0   )   byref  ->  zero-ref    V03._pointer(offs=0x00) P-INDEP "field V03._pointer (fldOffset=0x0)"
;* V11 tmp10        [V11    ] (  0,  0   )     int  ->  zero-ref    V03._length(offs=0x08) P-INDEP "field V03._length (fldOffset=0x8)"
;* V12 tmp11        [V12    ] (  0,  0   )   byref  ->  zero-ref    V04._value(offs=0x00) P-INDEP "field V04._value (fldOffset=0x0)"
;* V13 tmp12        [V13    ] (  0,  0   )   byref  ->  zero-ref    V05._pointer(offs=0x00) P-INDEP "field V05._pointer (fldOffset=0x0)"
;* V14 tmp13        [V14    ] (  0,  0   )     int  ->  zero-ref    V05._length(offs=0x08) P-INDEP "field V05._length (fldOffset=0x8)"
;
; Lcl frame size = 0

G_M14832_IG01:
						;; bbWeight=1    PerfScore 0.00
G_M14832_IG02:
       0FB7C1               movzx    rax, cx
       0FB6C0               movzx    rax, al
       4863C0               movsxd   rax, eax
       48BA7CB76ABD22020000 mov      rdx, 0x222BD6AB77C
       0FB60410             movzx    rax, byte  ptr [rax+rdx]
						;; bbWeight=1    PerfScore 3.00
G_M14832_IG03:
       C3                   ret      
						;; bbWeight=1    PerfScore 1.00

; Total bytes of code 24, prolog size 0, PerfScore 6.40

Here we have both the unnecessary movzx, as well as that extra movsxd rax, eax (presumably because the Unsafe.Add overload it picks up is the int one?) that I think the JIT should be able to remove as well, as it should be able to see that the value is immediately converted to native integer right after that, so that sign extension isn't necessary in this situation.

Thankfully this can be worked around by just manually casting to IntPtr, I couldn't remove that duplicate movzx though.

Configuration

• Tested on both .NET 5 Preview 6 with disasmo, and on sharplab.io on .NET Core 3.x
• Locally I have Windows 10 Pro x64 19041.x
• x64

Regression?

From what I can see on sharplab.io, this happens on .NET Core 3.x too, and even way back on .NET Framework x64.

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

[Sergio0694]

Possibly related, I also found this other issue that causes an unnecessary memory access.

Consider this method:

internal static bool TryParseOperator1(ref byte r0, out byte op)
{
    op = r0;

    return op != 0xFF;
}

Which is compiled to:

G_M26611_IG02:
       0FB601               movzx    rax, byte  ptr [rcx]
       8802                 mov      byte  ptr [rdx], al
       803AFF               cmp      byte  ptr [rdx], 255
       0F95C0               setne    al
       0FB6C0               movzx    rax, al
                        ;; bbWeight=1    PerfScore 6.25
G_M26611_IG03:
       C3                   ret 

You can see that cmp byte ptr [rdx], 255 shouldn't be there, it's like the JIT "forgets" it already has that value stored in rax, so it reads that from memory again. This can be worked around for now by doing this:

internal static bool TryParseOperator2(ref byte r0, out byte op)
{
    byte cmp = op = r0;

    return cmp != 0xFF;
}

Which gives the correct codegen here:

G_M41584_IG02:
       0FB601               movzx    rax, byte  ptr [rcx]
       8802                 mov      byte  ptr [rdx], al
       3DFF000000           cmp      eax, 255
       0F95C0               setne    al
       0FB6C0               movzx    rax, al
                        ;; bbWeight=1    PerfScore 4.50
G_M41584_IG03:
       C3                   ret  

With just one read from [rcx] and one write to [rdx]. Let me know if this is related (possibly caused by that initial movzx somehow, or whether I should open a second issue about this other thing specifically 😄

[BruceForstall]

cc @AndyAyersMS

[BruceForstall]

In the first case,

       0FB7C1               movzx    rax, cx
       0FB6C0               movzx    rax, al

The second extension is required (it zero extends a byte; the first zero extends a word), but the first can be eliminated, and we could have:

movzx rax, cl

After inlining the IntPtr conversion, before global morph, we have:

 *  ASG       long
 +--*  LCL_VAR   long   V03 tmp2
 \--*  CAST      long <- ulong <- uint
    \--*  CAST      int <- ubyte <- int
       \--*  LCL_VAR   ushort V00 arg0

after morph, we have:

*  ASG       long
+--*  LCL_VAR   long   V03 tmp2
\--*  CAST      long <- ulong <- uint
   \--*  CAST      int <- ubyte <- int
      \--*  CAST      int <- ushort <- int
         \--*  LCL_VAR   int    V00 arg0

The third (outermost) cast is eliminated by an xarch zero peephole optimization. fgMorphCast in post-order processing looks to see whether it has an operand GT_CAST that casts to the same type (and thus would be a duplicate cast). It could also check whether it is casting to a smaller type than the operand, in which case the operand cast could be removed.

Of course, a more general, late phase register bit-wise extraneous zero/sign extension phase could also be implemented, but we don't have much infrastructure for that.