JIT doesn't eliminate bounds checks sometimes

[EgorBo]

During dotnet/coreclr#21336 I noticed that JIT doesn't eliminate bounds checks in some cases despite the (uint) pattern, e.g.:

public void WriteBytes(Span<byte> destination)
{
    if ((uint)destination.Length >= 5)
    {
        destination[0] = 1;
        destination[1] = 2;
        destination[2] = 3;
        destination[3] = 4;
        destination[4] = 5;
    }
}

Output:

WriteBytes(System.Span`1)
  | cmp     edx,0
  | jbe     00007ffd`190b6d11
  | mov     byte ptr [rax],1
  | cmp     edx,1
  | jbe     00007ffd`190b6d11
  | mov     byte ptr [rax+1],2
  | cmp     edx,2
  | jbe     00007ffd`190b6d11
  | mov     byte ptr [rax+2],3
  | cmp     edx,3
  | jbe     00007ffd`190b6d11
  | mov     byte ptr [rax+3],4
  | cmp     edx,4
  | jbe     00007ffd`190b6d11
  | mov     byte ptr [rax+4],5
  | add     rsp,28h

but if I change the condition a bit - it might help:
if ((uint)destination.Length >= 5) - remain
if ((uint)destination.Length > 4) - eliminated!
if ((uint)destination.Length == 5) - eliminated!
if (5 <= (uint)destination.Length) - remain

Is it a known issue/feature-request?

TieredJit is disabled
.NET Core 3.0.0-preview-27122-01 (CoreCLR 4.6.27121.03, CoreFX 4.7.18.57103), 64bit RyuJIT

category:cq
theme:range-check
skill-level:intermediate
cost:small

[mikedn]

JIT doesn't eliminate bounds checks in some cases despite the (uint) pattern,

The unsigned compare shouldn't be needed in this case because both the constant and the array length are obviously positive. It's only needed when the index is a variable, basically it helps the JIT figure out that the variable is positive. Ideally the JIT should recognize something like (index >= 0 && index < a.Length) but it doesn't do that today.

Is it a known issue/feature-request?

I think I've seen this issue reported in the past, I'll have to check. Anyway, it doesn't hurt to have an additional example.

[mikedn]

So, something like if (4 < (uint)destination.Length) results in range check elimination but the similar signed comparison doesn't do that. It turns out that the reason why the unsigned comparison works is exactly the "hack" I added to the JIT to deal with if ((uint)i < (uint)destination.Length).

I don't think that at the time I considered the case of constant indices. Or I did but ignored it because it didn't seem common. I'll try to extend the existing logic and see if there are other hits in the framework to make this worthwhile.

[GSPP]

I thought I'd test a few variants of this pattern:

        void Test_Array_Constant(int[] array)
        {
            //signed:
            if (array.Length >= 5) array[4] = 0x1001;
            if (array.Length > 4) array[4] = 0x1002;
            if (array.Length == 5) array[4] = 0x1003;
            if (5 <= array.Length) array[4] = 0x1004;
            if (4 < array.Length) array[4] = 0x1005;

            //unsigned:
            if ((uint)array.Length >= (uint)5) array[4] = 0x1001;
            if ((uint)array.Length > (uint)4) array[4] = 0x1002; //range check eliminated
            if ((uint)array.Length == (uint)5) array[4] = 0x1003;
            if ((uint)5 <= (uint)array.Length) array[4] = 0x1004; //range check eliminated
            if ((uint)4 < (uint)array.Length) array[4] = 0x1005;
        }

        void Test_Array_Span(Span<int> array)
        {
            //signed:
            if (array.Length >= 5) array[4] = 0x1001;
            if (array.Length > 4) array[4] = 0x1002;
            if (array.Length == 5) array[4] = 0x1003; //range check eliminated (difference compared to array case)
            if (5 <= array.Length) array[4] = 0x1004;
            if (4 < array.Length) array[4] = 0x1005;

            //unsigned:
            if ((uint)array.Length >= (uint)5) array[4] = 0x1001;
            if ((uint)array.Length > (uint)4) array[4] = 0x1002; //range check eliminated
            if ((uint)array.Length == (uint)5) array[4] = 0x1003; //range check eliminated (difference compared to array case)
            if ((uint)5 <= (uint)array.Length) array[4] = 0x1004; //range check eliminated
            if ((uint)4 < (uint)array.Length) array[4] = 0x1005;
        }

Interestingly, the span version manages to eliminate more range checks. Also, using unsigned helps with some cases.

[am11]

I built the the latest master (30e643e) on macOS and tested with COMPlus_JitDisasm=WriteBytes COMPlus_TieredCompilation=0, it seems that some cases @EgorBo mentioned in top post are still not optimized:

expression	is bound check eliminated
* if ((uint)destination.Length >= 5)	no
if ((uint)destination.Length > 4)	yes
if ((uint)destination.Length <= 5)	no
if ((uint)destination.Length < 4)	no
if ((uint)destination.Length == 5)	yes
* if (5 <= (uint)destination.Length)	no
if (4 < (uint)destination.Length)	yes
if (5 >= (uint)destination.Length)	no
if (4 > (uint)destination.Length)	no
if (5 == (uint)destination.Length)	yes
if (destination.Length >= 5)	yes
if (destination.Length > 4)	yes
if (destination.Length <= 5)	no
if (destination.Length < 4)	no
if (destination.Length == 5)	yes
if (5 <= destination.Length)	yes
if (4 < destination.Length)	yes
if (5 >= destination.Length)	no
if (4 > destination.Length)	no
if (5 == destination.Length)	yes

disasm of > 4:

G_M58786_IG03:
       488B442408           mov      rax, bword ptr [rsp+08H]
       C60001               mov      byte  ptr [rax], 1
       C6400102             mov      byte  ptr [rax+1], 2
       C6400203             mov      byte  ptr [rax+2], 3
       C6400304             mov      byte  ptr [rax+3], 4
       C6400405             mov      byte  ptr [rax+4], 5
                                                ;; bbWeight=0.50 PerfScore 3.00

vs. >= 5:

G_M58786_IG03:
       85C0                 test     eax, eax
       7431                 je       SHORT G_M58786_IG05
       488B7DF0             mov      rdi, bword ptr [rbp-10H]
       C60701               mov      byte  ptr [rdi], 1
       83F801               cmp      eax, 1
       7625                 jbe      SHORT G_M58786_IG05
       C6470102             mov      byte  ptr [rdi+1], 2
       83F802               cmp      eax, 2
       761C                 jbe      SHORT G_M58786_IG05
       C6470203             mov      byte  ptr [rdi+2], 3
       83F803               cmp      eax, 3
       7613                 jbe      SHORT G_M58786_IG05
       C6470304             mov      byte  ptr [rdi+3], 4
       83F804               cmp      eax, 4
       760A                 jbe      SHORT G_M58786_IG05
       C6470405             mov      byte  ptr [rdi+4], 5
                                                ;; bbWeight=0.50 PerfScore 6.12

[nathan-moore]

@am11 I was mostly focused on the cases like (5 < destination.Length) without the cast, as I figured it would be more common/natural. I can see about this scenario if you disagree.

Unsigned comparisons are currently largely ignored by this area of assertion prop, barring the assertion that leads to the removal of the above checks.

[am11]

@nathan-moore, this is a great news. I have updated the table with cases without casts.

I have marked two cases with leading asterisk (*) which are interesting to fix because this pattern is common in the existing consumer code; as so far, this is considered as a (tricky) way to elide away the bound checks. The (uint)X >= # pattern can also be found in this repo with e.g. git grep "(uint).*>=" :/*.cs. We can cleanup some of those casts, which have been rendered redundant by your change.

[briansull]

Reopening this issue to address the remaining cases which aren't handled:

We should at least address these two common cases:

expression bounds check not eliminated

• if ((uint)destination.Length >= 5)
• if (5 <= (uint)destination.Length)