dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

Unnecessary LdelemaRef call not elided with Volatile.Read #65789

Closed stephentoub closed 1 year ago

stephentoub commented 2 years ago

Consider the code:

using System;
using System.Threading;

public sealed class C
{
    private readonly C[] _array;

    public C Get1() => Volatile.Read(ref _array[0]);

    public C Get2() => _array[0];
}

We shouldn't need a helper to read a value from the array, but even though the Volatile.Read evaporates on x86/64, we're still left with a non-inlined CastHelpers.LdelemaRef call in Get1:

; Core CLR 6.0.121.56705 on x86

C.Get1()
    L0000: mov ecx, [ecx+4]
    L0003: push 0x1171c6d8
    L0008: xor edx, edx
    L000a: call System.Runtime.CompilerServices.CastHelpers.LdelemaRef(System.Array, Int32, Void*)
    L000f: mov eax, [eax]
    L0011: ret

C.Get2()
    L0000: mov eax, [ecx+4]
    L0003: cmp dword ptr [eax+4], 0
    L0007: jbe short L000d
    L0009: mov eax, [eax+8]
    L000c: ret
    L000d: call 0x722c5fc0
    L0012: int3

; Core CLR 6.0.121.56705 on amd64

C.Get1()
    L0000: sub rsp, 0x28
    L0004: mov rcx, [rcx+8]
    L0008: xor edx, edx
    L000a: mov r8, 0x7ffaab0fccd0
    L0014: call 0x00007ffa9fda8200
    L0019: mov rax, [rax]
    L001c: add rsp, 0x28
    L0020: ret

C.Get2()
    L0000: sub rsp, 0x28
    L0004: mov rax, [rcx+8]
    L0008: cmp dword ptr [rax+8], 0
    L000c: jbe short L0017
    L000e: mov rax, [rax+0x10]
    L0012: add rsp, 0x28
    L0016: ret
    L0017: call 0x00007ffaffa6ec10
    L001c: int3

SharpLab

category:cq theme:volatile

ghost commented 2 years ago

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

Issue Details
Consider the code: ```C# using System; using System.Threading; public sealed class C { private readonly C[] _array; public C Get1() => Volatile.Read(ref _array[0]); public C Get2() => _array[0]; } ``` We shouldn't need a helper to read a value from the array, but even though the `Volatile.Read` evaporates on x86/64, we're still left with a non-inlined CastHelpers.LdelemaRef call in Get1: ``` ; Core CLR 6.0.121.56705 on x86 C.Get1() L0000: mov ecx, [ecx+4] L0003: push 0x1171c6d8 L0008: xor edx, edx L000a: call System.Runtime.CompilerServices.CastHelpers.LdelemaRef(System.Array, Int32, Void*) L000f: mov eax, [eax] L0011: ret C.Get2() L0000: mov eax, [ecx+4] L0003: cmp dword ptr [eax+4], 0 L0007: jbe short L000d L0009: mov eax, [eax+8] L000c: ret L000d: call 0x722c5fc0 L0012: int3 ; Core CLR 6.0.121.56705 on amd64 C.Get1() L0000: sub rsp, 0x28 L0004: mov rcx, [rcx+8] L0008: xor edx, edx L000a: mov r8, 0x7ffaab0fccd0 L0014: call 0x00007ffa9fda8200 L0019: mov rax, [rax] L001c: add rsp, 0x28 L0020: ret C.Get2() L0000: sub rsp, 0x28 L0004: mov rax, [rcx+8] L0008: cmp dword ptr [rax+8], 0 L000c: jbe short L0017 L000e: mov rax, [rax+0x10] L0012: add rsp, 0x28 L0016: ret L0017: call 0x00007ffaffa6ec10 L001c: int3 ``` [SharpLab](https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcAMACOSB0AKgBYBOApmACYCWAdgOYDc66cAzNhJQDblW4E2AMLoA3umxTsAB1I0AbmGDlsFagHs6PAJ4iA2gF1sAfTClSYHSzTTsk6RxHYA4uWBIAFAEpsAXgA+bAA1DR5lGj58ACVKKk8KADNTc0sdfUxDbxs7ByknYVd3BB9/ILMLKwzDGwBfIA=)
Author: stephentoub
Assignees: -
Labels: `area-CodeGen-coreclr`, `untriaged`
Milestone: -
EgorBo commented 2 years ago

Not related: we should make CastHelpers inlineable (for really hot paths) e.g. LdelemaRef

jkotas commented 2 years ago

From my understanding CORINFO_HELP_LDELEMA_REF can be eliminated if it's followed by dereference (IND)

ldelema does type check. It can be eliminated if you can reason about the exact type of the array. In this specific example, the JIT needs to take advantage of the fact that C is sealed and the array type is C[] and thus the type check is guaranteed to succeed. If C was not sealed, the type check can fail - for example:

using System;
using System.Threading;

public class C
{
    private readonly C[] _array = new D[1];

    public C Get1() => Volatile.Read(ref _array[0]);

    static void Main() { new C().Get1(); }
}

public class D : C { }
EgorBo commented 2 years ago

Sure! Good pont. Worth noting that even for the following case we currently also end up with an ldelema helper:

public sealed class C
{
    private readonly C[] _array;

    public C Get2()
    {
        ref C c = ref _array[0];
        return c;
    }
}

it also forward-substituted to IND(CORINFO_HELP_LDELEMA_REF)