Fix generic runtime lookup expansion in Tier0

[EgorBo]

Fixes #101131

Example:

[MethodImpl(MethodImplOptions.NoInlining)]
static void Foo<T>() => Bar<T>();

[MethodImpl(MethodImplOptions.NoInlining)]
static void Bar<T>() { }

Codegen diff for Foo<_Canon> in Tier0: https://www.diffchecker.com/nVRxo2tr/

tldr: it was an unfortunate "optimization" for Tier0 to reduce code size - instead of spilling a tree to stack and load from it - we used to do load from the context directly again, hence we did it twice - for nullcheck and for the fast path creating a race condition.

The bug is Tier0 only, e.g. here is the codege for Foo<_Canon> in Tier1:

; Assembly listing for method Prog:Foo[System.__Canon]() (FullOpts)
; FullOpts code
       sub      rsp, 40
       mov      qword ptr [rsp+0x20], rcx
       mov      rdx, qword ptr [rcx+0x38]
       mov      rdx, qword ptr [rdx+0x10]
       test     rdx, rdx
       je       SHORT G_M2381_IG04
       jmp      SHORT G_M2381_IG05
G_M2381_IG04:
       mov      rdx, 0x7FFE1F42D630 
       call     CORINFO_HELP_RUNTIMEHANDLE_METHOD
       mov      rdx, rax
G_M2381_IG05:
       mov      rcx, rdx
       call     [Prog:Bar[System.__Canon]()]
       nop      
       add      rsp, 40
       ret      

[dotnet-policy-service]

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

[EgorBo]

As expected, a couple of size regressions in Tier1: https://dev.azure.com/dnceng-public/public/_build/results?buildId=646176&view=ms.vss-build-web.run-extensions-tab

@davidwrighton could you please take a look at the assembly diff in the PR's description to confirm it's the same issue?

Do we need to backport this to .NET 8.0 ? or any preview branch of .NET 9.0

[AndyAyersMS]

This was new behavior in 8.0, right? Looks like the code moved and I can't quite follow it back to when it was introduced.

[EgorBo]

This was new behavior in 8.0, right? Looks like the code moved and I can't quite follow it back to when it was introduced.

It was introduced in #81635 in .NET 8.0, previously, lookup was expanded during import, afair, without this bug.

[davidwrighton]

I'm a bit curious what the if (sizeCheck) path does. Also, I don't see diffs for Tier1 in the link you sent, contact me tomorrow to point me at them. It seems like it's probably doing what I want, although the code expansion is unfortunate.

[EgorBo]

I'm a bit curious what the if (sizeCheck) path does.

I don't see any issues in it - it reusing a cached indirect as expected, it's a rare case when VM requires size check (sets sizeOffset field in CORINFO_RUNTIME_LOOKUP)

Also, I don't see diffs for Tier1 in the link you sent, contact me tomorrow to point me at them. It seems like it's probably doing what I want, although the code expansion is unfortunate.

The bug is Tier0 only and Tier1 is fine as is, does this match your impressions? E.g. the asm snippets you posted in #101131 are for Tier0 (unoptimized) codegen.

[davidwrighton]

@EgorBo your comment referred to Tier1 codegen... which I couldn't find any diffs for. For Tier0 yes, it appears like the changes are as expected. It's a bit unfortunate that this results in a spill to the stack, but such is the problem with Tier0 codegen.

[davidwrighton]

Also, yes this should probably be ported back to .NET 8.0. Its a stress bug there, and was reported by an internal Microsoft customer using .NET 8.0

[EgorBo]

@EgorBo your comment referred to Tier1 codegen...

Ah I see, I should've used a better wording - I wanted to show that Tier1 is fine and attached a snippet. While for Tier0 I attached a link to diffchecker - https://www.diffchecker.com/nVRxo2tr/ (left - Main, right - this PR)