Skip to content

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

Closed

Description

@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

Activity

  1. ghost added
    area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
    on Feb 22, 2021
  2. ghost added
    untriagedNew issue has not been triaged by the area owner
    on Feb 22, 2021
  3. changed the title [-]typeof(T).IsValueType not completely removed in some cases[/-] [+]typeof(T).IsValueType not completely unimpactful in some cases[/+] on Feb 22, 2021
  4. EgorBo commented on Feb 22, 2021

    @EgorBo
    Member

    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?

  5. stephentoub commented on Feb 22, 2021

    @stephentoub
    MemberAuthor

    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.

  6. AndyAyersMS commented on Feb 22, 2021

    @AndyAyersMS
    Member

    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.

  7. JulieLeeMSFT commented on Feb 25, 2021

    @JulieLeeMSFT
    Member

    @AndyAyersMS so no solution until we implement forward substitution?

  8. AndyAyersMS commented on Feb 26, 2021

    @AndyAyersMS
    Member

    Not that I know of, no.

  9. AndyAyersMS commented on Feb 26, 2021

    @AndyAyersMS
    Member

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

  10. removed
    untriagedNew issue has not been triaged by the area owner
    on Feb 26, 2021
  11. added this to the Future milestone on Feb 26, 2021
  12. ghost added
    in-prThere is an active PR which will close this issue when it is merged
    on Jan 13, 2022
  13. added a commit that references this issue on Feb 4, 2022
    99dcf5b
  14. ghost removed
    in-prThere is an active PR which will close this issue when it is merged
    on Feb 4, 2022
  15. ghost locked as resolved and limited conversation to collaborators on Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions