Bounds checks on array/span not eliminated after length check

[stephentoub]

I've got code similar to the following repro:

using System;
using System.Runtime.InteropServices;
using System.Runtime.CompilerServices;

public class C
{
    public static void Main() => new C().TryFormat(new char[4], out _);

    public bool TryFormat(Span<char> dst, out int charsWritten)
    {
        if (dst.Length >= 4)
        {
            dst[0] = 't';
            dst[1] = 'r';
            dst[2] = 'u';
            dst[3] = 'e';
            charsWritten = 4;
            return true;
        }
        charsWritten = 0;
        return false;
    }
}

I was hoping/expecting the bounds checks on each of those four writes to dst to be eliminated, but they’re not:

G_M404_IG02:
       488B02               mov      rax, bword ptr [rdx]
       8B5208               mov      edx, dword ptr [rdx+8]
       83FA04               cmp      edx, 4
       7C3C                 jl       SHORT G_M404_IG04
       83FA00               cmp      edx, 0
       7641                 jbe      SHORT G_M404_IG06
       66C7007400           mov      word  ptr [rax], 116
       83FA01               cmp      edx, 1
       7637                 jbe      SHORT G_M404_IG06
       66C740027200         mov      word  ptr [rax+2], 114
       83FA02               cmp      edx, 2
       762C                 jbe      SHORT G_M404_IG06
       66C740047500         mov      word  ptr [rax+4], 117
       83FA03               cmp      edx, 3
       7621                 jbe      SHORT G_M404_IG06
       66C740066500         mov      word  ptr [rax+6], 101
       41C70004000000       mov      dword ptr [r8], 4
       B801000000           mov      eax, 1

To work around that, I can use Unsafe.Add and MemoryMarshal.GetReference, e.g.

public bool TryFormat(Span<char> dst, out int charsWritten)
{
    if (dst.Length >= 4)
    {
        ref char c = ref MemoryMarshal.GetReference(dst);
        c = 't';
        Unsafe.Add(ref c, 1) = 'r';
        Unsafe.Add(ref c, 2) = 'u';
        Unsafe.Add(ref c, 3) = 'e';
        charsWritten = 4;
        return true;
    }
    charsWritten = 0;
    return false;
}

in which case I get the better:

G_M408_IG02:
       837A0804             cmp      dword ptr [rdx+8], 4
       7C27                 jl       SHORT G_M408_IG04
       488B02               mov      rax, bword ptr [rdx]
       66C7007400           mov      word  ptr [rax], 116
       66C740027200         mov      word  ptr [rax+2], 114
       66C740047500         mov      word  ptr [rax+4], 117
       66C740066500         mov      word  ptr [rax+6], 101
       41C70004000000       mov      dword ptr [r8], 4
       B801000000           mov      eax, 1

but it’d be nice not to have to use Unsafe for cases like this.

cc: @AndyAyersMS
Related: https://github.com/dotnet/coreclr/issues/12639

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

[mikedn]

but it’d be nice not to have to use Unsafe for cases like this.

This works too:

        if (3 < (uint)dst.Length)
        {
            dst[0] = 't';
            dst[1] = 'r';
            dst[2] = 'u';
            dst[3] = 'e';
            charsWritten = 4;
            return true;
        }
        charsWritten = 0;
        return false;

Hmm, maybe I can make it work without the cast to uint. That's normally needed to ensure that the index is positive but in the case of constants this is obvious.

[stephentoub]

This works too

Interesting, thanks. Yeah, I see (uint)dst.Length > 3 works, but (uint)dst.Length >= 4 does not. That seems... arbitrary :)

[mikedn]

but (uint)dst.Length >= 4 does not

Right, that's another thing that can probably be improved in the case of constants. For variables something like dst.Length >= x is not of much use, unless perhaps you then have dst[x - 1]. That's something that the RangeCheck phase should be able to handle but it's so focused on dealing with loops that it pretty much misses anything else.

[tgiphil]

Off-topic question: How was the disassembly of the method generated? Thanks in advance.

[stephentoub]

How was the disassembly of the method generated?

With a checked build of coreclr (e.g. build.cmd -checked -skiptests), and then when running the repro with it, setting the COMPlus_JitDisasm environment variable to the name of the method, e.g.

set COMPlus_JitDisasm=TryFormat

[erozenfeld]

@dotnet/jit-contrib

[gfoidl]

@EgorBo this issue is resolved and can be closed?

[EgorBo]

@gfoidl yes, I've just checked, thanks for pointing that out!