JIT: Range Checks for "(uint)i > cns" pattern

[EgorBo]

Closes #13464
Closes #1343

Example:

static ReadOnlySpan<byte> MySpan => new byte[] { 1, 2, 3, 4, 5 };

int GetElement(int index)
{
    if ((uint)index < (uint)MySpan.Length) // and all variations
        return MySpan[index];
    return 0;
}

Codegen diff:

; Method Program:GetElement(int):int:this
G_M56827_IG01:
-      sub      rsp, 40
G_M56827_IG02:
-      mov      eax, edx
-      cmp      rax, 5
-      jge      SHORT G_M56827_IG05
+      cmp      edx, 5
+      jae      SHORT G_M56827_IG05
G_M56827_IG03:
-      cmp      edx, 5
-      jae      SHORT G_M56827_IG07
       mov      eax, edx
       mov      rdx, 0xD1FFAB1E
       movzx    rax, byte  ptr [rax+rdx]
G_M56827_IG04:
-      add      rsp, 40
       ret      
G_M56827_IG05:
       xor      eax, eax
G_M56827_IG06:
-      add      rsp, 40
       ret      
-G_M56827_IG07:
-      call     CORINFO_HELP_RNGCHKFAIL
-      int3     
-; Total bytes of code: 51
+; Total bytes of code: 25

(uint) cast is still needed for index because it can be negative. Alternative pattern index >= 0 && index < MySpan.Length also works here.

For all of these variations bound checks are now eliminated (none of them are eliminated in .NET 6.0):

public class Tests
{
    static ReadOnlySpan<byte> MySpan => new byte[] { 1, 2, 3, 4, 5 };

    public static int Test1(int index)
    {
        if ((uint)index < MySpan.Length)
            return MySpan[index];

        return 0;
    }

    public static int Test2(int index)
    {
        if (MySpan.Length > (uint)index)
            return MySpan[index];

        return 0;
    }

    public static int Test3(int index)
    {
        if (MySpan.Length <= (uint)index)
            return 0;

        return MySpan[index];
    }

    public static int Test4(int index)
    {
        if ((uint)index >= MySpan.Length)
            return 0;

        return MySpan[index];
    }

    public static int Test5(int index)
    {
        if (index < 0 || index >= MySpan.Length)
            return 0;

        return MySpan[index];
    }

    public static int Test6(int index)
    {
        if (index >= 0 && index < MySpan.Length)
            return MySpan[index];

        return 0;
    }

    public static int Test7(int index)
    {
        if (index < 0)
            throw new Exception();

        if (index >= MySpan.Length)
            throw new ArgumentException();

        return MySpan[index];
    }

    public static int Test8(int index)
    {
        if ((uint)index < 2) // 2 is less than MySpan.Length
            return MySpan[index];

        return 0;
    }

    public static int Test9(int index)
    {
        if (index < 2 || index > 4) // e.g. we restrict index to be in [2..3] range despite the fact MySpan is fine with [0..4]
            throw new ArgumentException();

        return MySpan[index];
    }
}

Unrelated pattern that got fixed too:

int foo(uint i, int[] array)
{
    if (i < array.Length)
        return array[i];
    return 0;
}

if index is defined as unsigned - we don't need any casts to uint.

[ghost]

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

Issue Details

---

Closes #13464
Closes #1343

Example:

static ReadOnlySpan<byte> MySpan => new byte[] { 1, 2, 3, 4, 5 };

int GetElement(int index)
{
    if ((uint)index < MySpan.Length)
        return MySpan[index];
    return 0;
}

Codegen diff:

; Method Program:GetElement(int):int:this
G_M56827_IG01:
-      sub      rsp, 40
G_M56827_IG02:
-      mov      eax, edx
-      cmp      rax, 5
-      jge      SHORT G_M56827_IG05
+      cmp      edx, 5
+      jae      SHORT G_M56827_IG05
G_M56827_IG03:
-      cmp      edx, 5
-      jae      SHORT G_M56827_IG07
       mov      eax, edx
       mov      rdx, 0xD1FFAB1E
       movzx    rax, byte  ptr [rax+rdx]
G_M56827_IG04:
-      add      rsp, 40
       ret      
G_M56827_IG05:
       xor      eax, eax
G_M56827_IG06:
-      add      rsp, 40
       ret      
-G_M56827_IG07:
-      call     CORINFO_HELP_RNGCHKFAIL
-      int3     
-; Total bytes of code: 51
+; Total bytes of code: 25

Author:	EgorBo
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

As a side bonus it removes bound checks for this:

static int Foo(int[] array, int i)
{
    if ((uint)i < array.Length)
        return array[i];

    return 0;
}

Previously it needed (uint) cast for array.Length too. But it won't work if array is Span yet (but I am planning to handle it at some point - e.g. #62421 adds [Intrinsic] for Span.get_Length())

[EgorBo]

For all of these variations bound checks are now eliminated (none of them are eliminated in .NET 6.0):

public class Tests
{
    static ReadOnlySpan<byte> MySpan => new byte[] { 1, 2, 3, 4, 5 };

    public static int Test1(int index)
    {
        if ((uint)index < MySpan.Length)
            return MySpan[index];

        return 0;
    }

    public static int Test2(int index)
    {
        if (MySpan.Length > (uint)index)
            return MySpan[index];

        return 0;
    }

    public static int Test3(int index)
    {
        if (MySpan.Length <= (uint)index)
            return 0;

        return MySpan[index];
    }

    public static int Test4(int index)
    {
        if ((uint)index >= MySpan.Length)
            return 0;

        return MySpan[index];
    }

    public static int Test5(int index)
    {
        if (index < 0 || index >= MySpan.Length)
            return 0;

        return MySpan[index];
    }

    public static int Test6(int index)
    {
        if (index >= 0 && index < MySpan.Length)
            return MySpan[index];

        return 0;
    }

    public static int Test7(int index)
    {
        if (index < 0)
            throw new Exception();

        if (index >= MySpan.Length)
            throw new ArgumentException();

        return MySpan[index];
    }

    public static int Test8(int index)
    {
        if ((uint)index < 2) // 2 is less than MySpan.Length
            return MySpan[index];

        return 0;
    }

    public static int Test9(int index)
    {
        if (index < 2 || index > 4) // e.g. we restrict index to be in [2..3] range despite the fact MySpan is fine with [0..4]
            throw new ArgumentException();

        return MySpan[index];
    }
}

Unrelated pattern that got fixed too:

int foo(uint i, int[] array)
{
    if (i < array.Length)
        return array[i];
    return 0;
}

if index is defined as unsigned - we don't need any casts to uint.

[EgorBo]

/azp run Fuzzlyn, Antigen

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[EgorBo]

@dotnet/jit-contrib @SingleAccretion PTAL (Antigen failures are not related)

I generated a lot of test cases locally (~100k lines of code) e.g. https://gist.github.com/EgorBo/1ba16f0d9b1b7d87044c7ed4d14db117 to validate there are no surprises. But I don't think I should add them to the repo as they are too big/slow to run and don't add much value.

[EgorBo]

Diffs: https://dev.azure.com/dnceng/public/_build/results?buildId=1521711&view=ms.vss-build-web.run-extensions-tab

[EgorBo]

@dotnet/jit-contrib PTAL

[EgorBo]

@jakobbotsch can you take a look again? New diffs: https://dev.azure.com/dnceng/public/_build/results?buildId=1621068&view=ms.vss-build-web.run-extensions-tab

I inspected a couple of minor regressions and they seem to be caused by CSE/RA

[jakobbotsch]

LGTM, just had one question.
cc @SingleAccretion in case you want to give this a lookover as well.

[EgorBo]

@SingleAccretion thanks for the feedback!! addressed almost all of it except BashToNop - I'm leaving it to whoever will refactor it to optNarrowTree.

Failures are not related: #65818