JIT: Redundant bounds check when iterating over many spans with equal length

[BoyBaykiller]

This is like #114744 (which had nice diffs) but for Span. count of both Spans is equal so both bounds check could be elided, yet only the one thats used as the loop upper limit is.
https://godbolt.org/z/74aqo6Mn3

void TransformMesh(int[] first, float[] second, int start, int count)
{
    Span<int> vertexPositions = first.AsSpan(start, count);
    Span<float> vertices = second.AsSpan(start, count);

    for (int i = 0; i < vertices.Length; i++)
    {
        //vertices[i] = default;
        vertexPositions[i] = default; // redundant bounds check
    }
}

C:TransformMesh(int[],float[],int,int):this (FullOpts):
       push     rbp
       mov      rbp, rsp
       test     rsi, rsi
       je       G_M29678_IG13
       mov      eax, ecx
       mov      edi, r8d
       add      rax, rdi
       mov      edi, dword ptr [rsi+0x08]
       cmp      rax, rdi
       ja       SHORT G_M29678_IG12
       mov      eax, ecx
       lea      rax, bword ptr [rsi+4*rax+0x10]
       mov      edi, r8d
G_M29678_IG04:  ;; offset=0x0027
       test     rdx, rdx
       je       SHORT G_M29678_IG06
       mov      ecx, ecx
       mov      esi, r8d
       add      rcx, rsi
       mov      edx, dword ptr [rdx+0x08]
       cmp      rcx, rdx
       ja       SHORT G_M29678_IG12
       jmp      SHORT G_M29678_IG07
       align    [12 bytes for IG09]
G_M29678_IG06:  ;; offset=0x004A
       or       ecx, r8d
       jne      SHORT G_M29678_IG12
       xor      r8d, r8d
G_M29678_IG07:  ;; offset=0x0052
       xor      ecx, ecx
       test     r8d, r8d
       je       SHORT G_M29678_IG10
       cmp      r8d, edi
       jg       SHORT G_M29678_IG11
       xor      ecx, ecx
G_M29678_IG09:  ;; offset=0x0060
       xor      edi, edi
       mov      dword ptr [rax+rcx], edi
       add      rcx, 4
       dec      r8d
       jne      SHORT G_M29678_IG09
G_M29678_IG10:  ;; offset=0x006E
       pop      rbp
       ret      
G_M29678_IG11:  ;; offset=0x0070
       cmp      ecx, edi
       jae      SHORT G_M29678_IG14
       mov      edx, ecx
       xor      esi, esi
       mov      dword ptr [rax+4*rdx], esi
       inc      ecx
       cmp      ecx, r8d
       jl       SHORT G_M29678_IG11
       jmp      SHORT G_M29678_IG10
G_M29678_IG12:  ;; offset=0x0084
       call     [System.ThrowHelper:ThrowArgumentOutOfRangeException()]
       int3     
G_M29678_IG13:  ;; offset=0x008B
       mov      eax, ecx
       or       eax, r8d
       jne      SHORT G_M29678_IG12
       xor      rax, rax
       xor      edi, edi
       jmp      SHORT G_M29678_IG04
G_M29678_IG14:  ;; offset=0x0098
       call     CORINFO_HELP_RNGCHKFAIL
       int3     

[dotnet-policy-service]

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

[MihuBot]

I'm a bot. Here is a possible related and/or duplicate issue (I may be wrong):

• No way to avoid bounds checks when multiple spans are involved #97285

[EgorBo]

looks like there are no bound checks to me

[En3Tho]

@EgorBo I do see some loop cloning here but technically a loop woth bounds check shouldn't be needed? Slice throws an exception when start + length doesn't fit in the original span (and it's also inlined here). So maybe this information can be propagated somehow?

[BoyBaykiller]

Also note when I use count as the loop upper limit like this there are two bounds checks:

static void TransformMesh(int[] first, float[] second, int start, int count)
{
    Span<int> vertexPositions = first.AsSpan(start, count);
    Span<float> vertices = second.AsSpan(start, count);

    for (int i = 0; i < count; i++)
    {
        vertices[i] = default; // bounds check
        vertexPositions[i] = default; // bounds check
    }
}

[En3Tho]

@BoyBaykiller Your second example is exactly as the first one: there is a core loop with no bound checks:

G_M60160_IG09:  ;; offset=0x005D
       xor      esi, esi
       mov      dword ptr [rax+rdx], esi
       mov      dword ptr [rdi+rdx], esi
       add      rdx, 4
       dec      ecx
       jne      SHORT G_M60160_IG09

But again, another loop with bound checks should not be needed because AsSpan will throw if inputs do not match the length requirement

[BoyBaykiller]

Yep thats what I mean, the slow path is not needed. Bounds Checks can be statically optimized away.
In the real world loop cloning often does not kick in (mainly because loop too large) and then there's only the slow path.

[BoyBaykiller]

Ok, the issue here has to do with how AsSpan is implemented. The length of the returned Span may be 0:

runtime/src/libraries/System.Private.CoreLib/src/System/Span.cs

Lines 70 to 76 in 44e1a20

	if (array == null)
	{
	if (start != 0 || length != 0)
	ThrowHelper.ThrowArgumentOutOfRangeException();
	this = default;
	return; // returns default
	}

So handling the cases I've shown here with iteration over multiple Spans is not expected to be handled by JIT, because one span may have Length == 0 while the other has Length == count. If you want to get rid of the bounds checks you have to assert either of:

1. array non null
2. start non 0
3. count non 0

Or make your own AsSpan extension that throws System.NullReferenceException instead of returning 0 when array == null:

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Span<T> AsSpan<T>(this T[] array, int start, int length)
{
    // This exists because: https://github.com/dotnet/runtime/issues/122696#issuecomment-3688365051

    ArgumentOutOfRangeException.ThrowIfGreaterThan((ulong)(uint)start + (ulong)(uint)length, (ulong)(uint)array.Length);

    return MemoryMarshal.CreateSpan(ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(array), (nint)(uint)start), length);
}

[BoyBaykiller]

This case where only one span is iterated could still be handled though:

void TransformMesh(int[] array, int start, int count)
{
    Span<int> span = array.AsSpan(start, count);
    for (int i = 0; i < count; i++)
    {
        span[i] = i; // bounds check
    }
}

Either:

1. span.Length == count and we can elide the bounds check or
2. array was null and span.Length == 0 then we don't need a bounds check either

Actually I think this should work with many spans too...

[BoyBaykiller]

Anyway. @EgorBo The "fixed repro" is:

static void Test(Span<int> first, Span<int> second, int count)
{
    Span<int> spanA = first.Slice(0, count);
    Span<int> spanB = second.Slice(0, count);

    for (int i = 0; i < count; i++)
    {
        spanA[i] = i;
        spanB[i] = i;
    }
}

The difference is that the Span is created from an existing Span instead of array to avoid problems with length possibly being 0 (see above).
This case is already handled so will close.

[En3Tho]

@BoyBaykiller Interesting. Thamk you for these findings. One of the ways to preserve the original signature would be calling AsSpan + Slice as separate operations

static void TransformMesh3(int[] first, float[] second, int start, int count)
{
    Span<int> vertexPositions = first.AsSpan().Slice(start, count);
    Span<float> vertices = second.AsSpan().Slice(start, count);

    for (int i = 0; i < count; i++)
    {
        vertices[i] = default;
        vertexPositions[i] = default;
    }
}