typeof(T).IsValueType not completely unimpactful in some cases

[stephentoub]

The JIT has an optimization to treat typeof(T).IsValueType as a const based on the T being specified, but in some cases the presence of the check is still having an impact on the generated asm.

Non-sensical repro (derived from much more complicated real-world scenario in dotnet/roslyn#51383):

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

[DisassemblyDiagnoser]
public class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromAssemblies(new[] { typeof(Program).Assembly }).Run(args);
}

[DisassemblyDiagnoser]
public class Benchmarks
{
    private Wrapper<int> _array = new Wrapper<int>(new int[1]);
    private int _offset = 0;

    [Benchmark]
    public void Test1() => _array.Get1(_offset) = default;

    [Benchmark]
    public void Test2() => _array.Get2(_offset) = default;
}

public struct Wrapper<T>
{
    private T[] _items;

    public Wrapper(T[] item) => _items = item;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public ref T Get1(int index)
    {
        return ref _items[index];
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public ref T Get2(int index)
    {
        if (typeof(T).IsValueType)
        {
            return ref _items[index];
        }

        return ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(_items), index);
    }
}

The non-value type branch in Get2 should in theory be removed completely, but using a recent .NET 6 nightly, this results in:

.NET 6.0.0 (6.0.21.11705), X64 RyuJIT

; Benchmarks.Test1()
       sub       rsp,28
       lea       rax,[rcx+10]
       mov       edx,[rcx+8]
       mov       rax,[rax]
       cmp       edx,[rax+8]
       jae       short M00_L00
       movsxd    rdx,edx
       xor       ecx,ecx
       mov       [rax+rdx*4+10],ecx
       add       rsp,28
       ret
M00_L00:
       call      CORINFO_HELP_RNGCHKFAIL
       int       3
; Total bytes of code 39

.NET 6.0.0 (6.0.21.11705), X64 RyuJIT

; Benchmarks.Test2()
       sub       rsp,28
       lea       rax,[rcx+10]
       mov       edx,[rcx+8]
       mov       rax,[rax]
       cmp       edx,[rax+8]
       jae       short M00_L00
       movsxd    rdx,edx
       lea       rax,[rax+rdx*4+10]
       xor       edx,edx
       mov       [rax],edx
       add       rsp,28
       ret
M00_L00:
       call      CORINFO_HELP_RNGCHKFAIL
       int       3
; Total bytes of code 42

Note the difference between:

       xor       ecx,ecx
       mov       [rax+rdx*4+10],ecx

and

       lea       rax,[rax+rdx*4+10]
       xor       edx,edx
       mov       [rax],edx

category:cq
theme:optimization
skill-level:expert
cost:large
impact:medium

[EgorBo]

The non-value type branch in Get2 should in theory be removed completely, but using a recent .NET 6 nightly, this results in:

Well, it's still removed, it's just some extra lea - perhaps related to some of the recent struct improvements work?

[stephentoub]

Well, it's still removed, it's just some extra lea

Right, I meant its impact should be completely removed. But it still has an impact.

[AndyAyersMS]

This is likely another example highlighting the need for forward substitution in the JIT (#6973).

The statement that gets removed also breaks up a tree and so inhibits tree-based optimizations like address mode building. We don't have anything that will try and glue broken trees back together.

[JulieLeeMSFT]

@AndyAyersMS so no solution until we implement forward substitution?

[AndyAyersMS]

Not that I know of, no.

[AndyAyersMS]

I don't expect us to get to this in .Net 6. So marking as future.