Compiler could generate better IL for collection expressions

[Neme12]

Version Used: current main on sharplab (19 Jan 2024, it doesn't say the version or commit 😔)

Steps to Reproduce:

For this code:

List<int> M()
{
    return [1, 2, 3, 4];
}

the compiler currently emits the equivalent of this:

List<int> M()
{
    List<int> list = new List<int>();
    CollectionsMarshal.SetCount(list, 4);
    Span<int> span = CollectionsMarshal.AsSpan(list);
    int num = 0;
    span[num] = 1;
    num++;
    span[num] = 2;
    num++;
    span[num] = 3;
    num++;
    span[num] = 4;
    num++;
    return list;
}

(sharplab)

I think it would be better if it generated the equivalent of this instead:

List<int> M()
{
    List<int> list = new List<int>();
    CollectionsMarshal.SetCount(list, 4);
    Span<int> span = CollectionsMarshal.AsSpan(list);
    ref int num = ref MemoryMarshal.GetReference(span);
    num = 1;
    num = ref Unsafe.Add(ref num, 1);        
    num = 2;
    num = ref Unsafe.Add(ref num, 1);        
    num = 3;
    num = ref Unsafe.Add(ref num, 1);
    num = 4;
    num = ref Unsafe.Add(ref num, 1);
    return list;
}

or, as an alternative:

List<int> M()
{
    List<int> list = new List<int>();
    CollectionsMarshal.SetCount(list, 4);
    Span<int> span = CollectionsMarshal.AsSpan(list);
    ref int num = ref MemoryMarshal.GetReference(span);
    Unsafe.Add(ref num, 0) = 1;
    Unsafe.Add(ref num, 1) = 2;
    Unsafe.Add(ref num, 2) = 3;
    Unsafe.Add(ref num, 3) = 4;
    return list;
}

Why?

1. It is more efficient, because the JIT currently doesn't elide range checks in the current codegen. You can see this in the sharplab example.by switching to assembler. This could of course be fixed (and probably should be), but I would argue that the compiler should emit the simpler code anyway, to not provide any chance whatsoever for the JIT to fail to do that.
2. It results in a lot less IL. This is important in today's age when .NET runs in environments like the browser. You can see this on sharplab - 46 lines of IL vs 29 and 27, respectively (and the resulting x64 asm is 55, 47 and 45 lines).

I understand that the compiler doesn't usually do optimizations and leaves that to the JIT, but:

1. In this case, the compiler doesn't need to do any complicated (or even simple) analysis to generate the desired code. It would simply switch from one codegen strategy to another, both of which have equal complexity.
2. One could argue that even the current codegen strategy is an optimization - to generate better code than repeatedly calling list.Add. And the compiler uses the Unsafe.Add approach for collection literals for spans today already.

It would also be nice to get rid of the last, unused num++; or num = ref Unsafe.Add(ref num, 1);. The JIT elides this of course, but as I mentioned, generating less IL is better, and in this case that last increment is simply redundant.

[RikkiGibson]

it doesn't say the version or commit

Clicking the little ^ next to the date will reveals it as d8d7886

[RikkiGibson]

It is more efficient, because the JIT currently doesn't elide range checks in the current codegen

So for the suggested codegen, is it part of the idea that by using Unsafe APIs on the underlying reference of the Span, there isn't even a range check to elide?

As far as which "variant" of the codegen to choose, a microbenchmark might help.

[RikkiGibson]

I would review a fix if accompanied by a microbenchmark of the current and proposed codegen.

[DoctorKrolic]

This is a compeating issue with #71183, which proposes just to use constant indicies instead of a variable. I've measured both approaches on .NET 10 and got the following results:

BenchmarkDotNet v0.14.0, Windows 11 (10.0.26100.3775)
AMD Ryzen 7 5800X, 1 CPU, 16 logical and 8 physical cores
.NET SDK 10.0.100-preview.3.25201.16
  [Host]     : .NET 10.0.0 (10.0.25.17105), X64 RyuJIT AVX2
  DefaultJob : .NET 10.0.0 (10.0.25.17105), X64 RyuJIT AVX2


| Method          | Mean     | Error     | StdDev    |
|---------------- |---------:|----------:|----------:|
| CurrentLowering | 8.202 ns | 0.1426 ns | 0.1264 ns |
| ConstantIndices | 7.447 ns | 0.0374 ns | 0.0312 ns |
| UnsafeIteration | 7.568 ns | 0.0664 ns | 0.0621 ns |

Full benchmark code

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

BenchmarkRunner.Run<Benchmarks>();

public class Benchmarks
{
    [Benchmark]
    public List<int> CurrentLowering()
    {
        return [1, 2, 3, 4];
    }

    [Benchmark]
    public List<int> ConstantIndices()
    {
        List<int> list = new List<int>();
        CollectionsMarshal.SetCount(list, 4);
        Span<int> span = CollectionsMarshal.AsSpan(list);
        span[0] = 1;
        span[1] = 2;
        span[2] = 3;
        span[3] = 4;
        return list;
    }

    [Benchmark]
    public List<int> UnsafeIteration()
    {
        List<int> list = new List<int>();
        CollectionsMarshal.SetCount(list, 4);
        Span<int> span = CollectionsMarshal.AsSpan(list);
        ref int num = ref MemoryMarshal.GetReference(span);
        Unsafe.Add(ref num, 0) = 1;
        Unsafe.Add(ref num, 1) = 2;
        Unsafe.Add(ref num, 2) = 3;
        Unsafe.Add(ref num, 3) = 4;
        return list;
    }
}

It is very clear, that unsafe approach doesn't have any benifits over just using constant indices. Therefore I believe we should close this in favour of #71183 since less unsafe = better