Skip to content
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: Don't convert GT/LE to EQ/NE for "ARR_LEN cmp 0" #35758

Closed
wants to merge 1 commit into from

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented May 2, 2020

RyuJIT converts (uint)x > 0 to x != 0 (same for <=) but I suggest we keep it as it is for things like (uint)array.Length > 0 since != and == operators are not bound checks elimination friendly, e.g.:

public static void Foo1(int[] array)
{
    if ((uint)array.Length > 0)
        array[0] = 42;
}

public static void Foo2(int[] array)
{
    if (0 < (uint)array.Length) // same condition, different order
        array[0] = 42;
}

Before my changes

; Method Egor:Foo1(System.Int32[])
       4883EC28             sub      rsp, 40
       8B4108               mov      eax, dword ptr [rcx+8]
       85C0                 test     eax, eax
       740C                 je       SHORT G_M52406_IG04
       83F800               cmp      eax, 0
       760C                 jbe      SHORT G_M52406_IG05
       C741102A000000       mov      dword ptr [rcx+16], 42
G_M52406_IG04:
       4883C428             add      rsp, 40
       C3                   ret      
G_M52406_IG05:
       E81FC5485F           call     CORINFO_HELP_RNGCHKFAIL
       CC                   int3     
; Total bytes of code: 34


; Method Egor:Foo2(System.Int32[])
       8B4108               mov      eax, dword ptr [rcx+8]
       85C0                 test     eax, eax
       7407                 je       SHORT G_M52789_IG04
       C741102A000000       mov      dword ptr [rcx+16], 42
G_M52789_IG04:
       C3                   ret      
; Total bytes of code: 15

^ As you can see, bound checks are removed only for "unnatural" (IMO) condition in Foo3
That's why we have this "unnatural" conditions in String.IsNullOrEmpty and other places.

After my changes

; Method Egor:Foo1(System.Int32[])
       8B4108               mov      eax, dword ptr [rcx+8]
       85C0                 test     eax, eax
       7407                 je       SHORT G_M52406_IG04
       C741102A000000       mov      dword ptr [rcx+16], 42
G_M52406_IG04:
       C3                   ret      
; Total bytes of code: 15


; Method Egor:Foo2(System.Int32[])
       8B4108               mov      eax, dword ptr [rcx+8]
       85C0                 test     eax, eax
       7407                 je       SHORT G_M52789_IG04
       C741102A000000       mov      dword ptr [rcx+16], 42
G_M52789_IG04:
       C3                   ret      
; Total bytes of code: 15

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 2, 2020
@benaadams
Copy link
Member

Span<int> has same issue

public static void Foo1(Span<int> array)
{
    if ((uint)array.Length > 0)
        array[0] = 42;
}
public static void Foo2(Span<int> array)
{
    if (0 < (uint)array.Length) // same condition, different order
        array[0] = 42;
}
C.Foo1(System.Span`1<Int32>)
    L0000: push ebp
    L0001: mov ebp, esp
    L0003: mov eax, [ebp+0xc]
    L0006: test eax, eax
    L0008: jz L0018
    L000a: cmp eax, 0x0
    L000d: jbe L001c
    L000f: mov eax, [ebp+0x8]
    L0012: mov dword [eax], 0x2a
    L0018: pop ebp
    L0019: ret 0x8
    L001c: call 0x62022680
    L0021: int3

C.Foo2(System.Span`1<Int32>)
    L0000: mov eax, [esp+0x8]
    L0004: test eax, eax
    L0006: jz L0012
    L0008: mov eax, [esp+0x4]
    L000c: mov dword [eax], 0x2a
    L0012: ret 0x8

{
// IL doesn't have a cne instruction so compilers use cgt.un instead. The JIT
// recognizes certain patterns that involve GT_NE (e.g (x & 4) != 0) and fails
// if GT_GT is used instead. Transform (x GT_GT.unsigned 0) into (x GT_NE 0)
// and (x GT_LE.unsigned 0) into (x GT_EQ 0). The later case is rare, it sometimes
// occurs as a result of branch inversion.
// NOTE: keep it as it is for Array.Length op1 since GT_NE/GT_EQ operators are
// not bounds check elimination friendly yet.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how hard it is to make it friendly?
As I see we use

static bool OperIsBoundsCheck(genTreeOps op)
to find what we optimize as a bound check and we don't have EQ/NE there, should we add them?

@EgorBo
Copy link
Member Author

EgorBo commented Sep 10, 2020

As far as I understand it's already handled in #40180

@EgorBo EgorBo closed this Sep 10, 2020
@nathan-moore
Copy link
Contributor

As far as I understand it's already handled in #40180

I checked, it indeed is :). Should also handle the span case.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants