New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RyuJIT fails to preserve variable allocated to RCX around shift on x64 in release #18884

Closed
jakobbotsch opened this Issue Jul 12, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@jakobbotsch
Collaborator

jakobbotsch commented Jul 12, 2018

On Linux, the following example prints a weird result:

public class Program
{
    static ushort s_3;
    static long s_5;
    public static void Main()
    {
        s_3 = 0; // avoid runtime checks in M15
        M15(0, 0, 1, 0);
    }

    static short[] M15(ulong arg0, long arg1, ushort arg2, byte arg3)
    {
        s_5 >>= 50 / arg2;  // the value shifted by here
        if (arg3 != 0)
        {
            s_3 = s_3;
        }

        System.Console.WriteLine(arg3); // ..is printed here
        return new short[]{0};
    }
}

The disassembly shows that arg3 gets allocated to RCX, but it is not restored after the shift (which requires the shift-value to be in cl):

; Assembly listing for method Program:M15(long,long,ushort,ubyte):ref
; Emitting BLENDED_CODE for X64 CPU with AVX
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;* V00 arg0         [V00    ] (  0,  0   )    long  ->  zero-ref
;* V01 arg1         [V01    ] (  0,  0   )    long  ->  zero-ref
;  V02 arg2         [V02,T02] (  3,  3   )  ushort  ->  rdx
;  V03 arg3         [V03,T01] (  4,  4   )   ubyte  ->  rcx         ; allocated to rcx
;# V04 OutArgs      [V04    ] (  1,  1   )  lclBlk ( 0) [rsp+0x00]
;  V05 cse0         [V05,T00] (  6,  6   )     int  ->  rdi
;
; Lcl frame size = 8

G_M58758_IG01:
       50                   push     rax

G_M58758_IG02:
       B832000000           mov      eax, 50
       0FB7FA               movzx    rdi, dx
       99                   cdq
       F7FF                 idiv     edx:eax, edi
       48BFC046295C0D7F0000 mov      rdi, 0x7F0D5C2946C0
       8BC8                 mov      ecx, eax
       48D33F               sar      qword ptr [rdi], cl
       400FB6F9             movzx    rdi, cl              ; arg3 reloaded from shift value
       85FF                 test     edi, edi
       741A                 je       SHORT G_M58758_IG03
       48B8C846295C0D7F0000 mov      rax, 0x7F0D5C2946C8
       0FB700               movzx    rax, word  ptr [rax]
       48BEC846295C0D7F0000 mov      rsi, 0x7F0D5C2946C8
       668906               mov      word  ptr [rsi], ax

G_M58758_IG03:
       E896FBFFFF           call     System.Console:WriteLine(int)
       48BFB048625D0D7F0000 mov      rdi, 0x7F0D5D6248B0
       BE01000000           mov      esi, 1
       E81A148A78           call     CORINFO_HELP_NEWARR_1_VC
       90                   nop

G_M58758_IG04:
       4883C408             add      rsp, 8
       C3                   ret

; Total bytes of code 92, prolog size 1 for method Program:M15(long,long,ushort,ubyte):ref
; ============================================================

It repros on Windows as well (see next comment).

@jakobbotsch jakobbotsch changed the title from RyuJIT fails to preserve variable allocated to RCX around shift on Linux x64 in release to RyuJIT fails to preserve variable allocated to RCX around shift on x64 in release Jul 12, 2018

@jakobbotsch

This comment has been minimized.

Show comment
Hide comment
@jakobbotsch

jakobbotsch Jul 12, 2018

Collaborator

Repro on Windows:

public class Program
{
    static ushort s_3;
    static long s_5;
    public static void Main()
    {
        s_3 = 0; // avoid runtime checks in M15
        M15(0, 0, 1, 0);
    }

    static short[] M15(byte arg0, long arg1, ushort arg2, ulong arg3)
    {
        s_5 >>= 50 / arg2;  // the value shifted by here
        if (arg0 != 0)
        {
            s_3 = s_3;
        }

        System.Console.WriteLine(arg0); // ..is printed here
        return new short[]{0};
    }
}

The difference was just in the calling-convention. In Windows, the first arg is in RCX. On Linux, the 4th arg is in RCX.

Collaborator

jakobbotsch commented Jul 12, 2018

Repro on Windows:

public class Program
{
    static ushort s_3;
    static long s_5;
    public static void Main()
    {
        s_3 = 0; // avoid runtime checks in M15
        M15(0, 0, 1, 0);
    }

    static short[] M15(byte arg0, long arg1, ushort arg2, ulong arg3)
    {
        s_5 >>= 50 / arg2;  // the value shifted by here
        if (arg0 != 0)
        {
            s_3 = s_3;
        }

        System.Console.WriteLine(arg0); // ..is printed here
        return new short[]{0};
    }
}

The difference was just in the calling-convention. In Windows, the first arg is in RCX. On Linux, the 4th arg is in RCX.

@RussKeldorph RussKeldorph added this to the 3.0 milestone Jul 13, 2018

@AndyAyersMS

This comment has been minimized.

Show comment
Hide comment
@AndyAyersMS

AndyAyersMS Jul 13, 2018

Member

Seems likely this is an RA issue... @CarolEidt PTAL
cc @dotnet/jit-contrib

Member

AndyAyersMS commented Jul 13, 2018

Seems likely this is an RA issue... @CarolEidt PTAL
cc @dotnet/jit-contrib

@CarolEidt

This comment has been minimized.

Show comment
Hide comment
@CarolEidt

CarolEidt Jul 14, 2018

Member

Thanks @jakobbotsch - I'll look at this!

Member

CarolEidt commented Jul 14, 2018

Thanks @jakobbotsch - I'll look at this!

@CarolEidt CarolEidt self-assigned this Jul 14, 2018

CarolEidt added a commit to CarolEidt/coreclr that referenced this issue Jul 16, 2018

Kill RCX when used by shift
RCX must be explicitly killed. Otherwise, if there's a case of a def/use conflict - as in this test case where the shift amount is defined by a divide that must go in RAX, it won't be explicitly assigned to RCX,.

Fix #18884

CarolEidt added a commit to CarolEidt/coreclr that referenced this issue Jul 17, 2018

Kill RCX when used by shift
RCX must be explicitly killed. Otherwise, if there's a case of a def/use conflict - as in this test case where the shift amount is defined by a divide that must go in RAX, it won't be explicitly assigned to RCX,.
Also, the handling of conflicts must not use the register assignment of the def on the use  if it conflicts with the use register requirements, and vice versa.

Fix #18884

@CarolEidt CarolEidt closed this in #18941 Jul 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment