JIT: inline shared generics with runtime lookups inside by EgorBo · Pull Request #99265 · dotnet/runtime

EgorBo · 2024-03-04T21:21:17Z

Closes #81432
Closes #8662
Closes #65815
(probably more issues, need to check)

Based on #81635.

This PR implements @jkotas's idea to inline shared generics which require runtime lookups.
Example:

[MethodImpl(MethodImplOptions.NoInlining)]
bool Test<T>() => Callee<T>();

bool Callee<T>() => typeof(T).IsValueType;

Codegen for Test<string> in Main:

; Assembly listing for method Program:Test[System.__Canon]():bool:this
       push     rbx
       sub      rsp, 48
       mov      qword ptr [rsp+0x28], rdx
       mov      rbx, rcx
       mov      rcx, qword ptr [rdx+0x10]
       mov      rax, qword ptr [rcx+0x10]
       test     rax, rax
       je       SHORT G_M000_IG04
       mov      rdx, rax
       jmp      SHORT G_M000_IG05
G_M000_IG04:
       mov      rcx, rdx
       mov      rdx, 0x7FFCC5FE7088
       call     CORINFO_HELP_RUNTIMEHANDLE_METHOD
       mov      rdx, rax
G_M000_IG05:
       mov      rcx, rbx
       call     [Program:Callee[System.__Canon]():bool:this]   ;;;  <---- not inlined
       nop      
       add      rsp, 48
       pop      rbx
       ret      
; Total bytes of code 68

Codegen for Test<string> in this PR:

; Assembly listing for method Program:Test[System.__Canon]():ubyte:this
       xor      eax, eax
       ret

I think the JIT side is ok (I just need to double check various "this context load is invariant" places), but I am not sure I implemented the VM side (only CoreCLR for now) correctly.

ghost · 2024-03-04T21:21:29Z

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

Issue Details

Closes #81432

This PR implements @jkotas's idea to inline shared generics which require runtime lookups.
Example:

[MethodImpl(MethodImplOptions.NoInlining)]
bool Test<T>() => Callee<T>();

bool Callee<T>() => typeof(T).IsValueType;

Codegen for Test<string> in Main:

; Assembly listing for method Program:Test[System.__Canon]():bool:this
       push     rbx
       sub      rsp, 48
       mov      qword ptr [rsp+0x28], rdx
       mov      rbx, rcx
       mov      rcx, qword ptr [rdx+0x10]
       mov      rax, qword ptr [rcx+0x10]
       test     rax, rax
       je       SHORT G_M000_IG04
       mov      rdx, rax
       jmp      SHORT G_M000_IG05
G_M000_IG04:
       mov      rcx, rdx
       mov      rdx, 0x7FFCC5FE7088
       call     CORINFO_HELP_RUNTIMEHANDLE_METHOD
       mov      rdx, rax
G_M000_IG05:
       mov      rcx, rbx
       call     [Program:Callee[System.__Canon]():bool:this]   ;;;  <---- not inlined
       nop      
       add      rsp, 48
       pop      rbx
       ret      
; Total bytes of code 68

Codegen for Test<string> in this PR:

; Assembly listing for method Program:Test[System.__Canon]():ubyte:this
       xor      eax, eax
       ret

I think the JIT side is ok (I just need to double check various "this context load is invariant" places), but I am not sure I implemented the VM side (only CoreCLR for now) correctly.

Author:	EgorBo
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

EgorBo · 2024-03-05T03:02:26Z

/azp run runtime-coreclr outerloop, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-03-05T03:02:49Z

Azure Pipelines successfully started running 2 pipeline(s).

EgorBo · 2024-03-05T14:05:19Z

@MihuBot

EgorBo · 2024-03-09T02:32:10Z

@jkotas one thing I struggle with currently is when I have tokenContext being class (CORINFO_CONTEXTFLAGS_CLASS) -- how do I know whether to use CORINFO_LOOKUP_CLASSPARAM or CORINFO_LOOKUP_THISOBJ ?

jkotas · 2024-03-09T02:53:48Z

when I have tokenContext being class (CORINFO_CONTEXTFLAGS_CLASS) -- how do I know whether to use CORINFO_LOOKUP_CLASSPARAM or CORINFO_LOOKUP_THISOBJ

It is not possible to tell from CORINFO_CONTEXTFLAGS_CLASS context alone. You need to have the current method to figure it out.

EgorBo · 2024-03-09T02:57:02Z

when I have tokenContext being class (CORINFO_CONTEXTFLAGS_CLASS) -- how do I know whether to use CORINFO_LOOKUP_CLASSPARAM or CORINFO_LOOKUP_THISOBJ

It is not possible to tell from CORINFO_CONTEXTFLAGS_CLASS context alone. You need to have the current method to figure it out.

Current method as in the one we currently inline, right? not the m_methodBeingCompiled (which is the root method). So I presume I need to extend the JIT-EE interface

EgorBo · 2024-03-09T02:59:07Z

Although, I am not sure I understand where should I pass it, ComputeRuntimeLookupForSharedGenericToken is not a direct jit-ee api..

jakobbotsch

JIT side LGTM.

MichalStrehovsky

The ilc and crossgen2 (to the extent that I understand crossgen2) parts lgtm modulo the question.

MichalStrehovsky · 2024-03-13T06:30:51Z

        }

-        private bool getReadyToRunHelper(ref CORINFO_RESOLVED_TOKEN pResolvedToken, ref CORINFO_LOOKUP_KIND pGenericLookupKind, CorInfoHelpFunc id, ref CORINFO_CONST_LOOKUP pLookup)
+        private bool getReadyToRunHelper(ref CORINFO_RESOLVED_TOKEN pResolvedToken, ref CORINFO_LOOKUP_KIND pGenericLookupKind, CorInfoHelpFunc id, CORINFO_METHOD_STRUCT_* callerHandle, ref CORINFO_CONST_LOOKUP pLookup)


I'm curious why callerHandle can be unused here (we use pResolvedToken.tokenContext to get the context instead), but embedGenericHandle and getCallInfo need the callerHandle and don't use context. Since the tests are passing, maybe callerHandle is redundant with tokenContext?

Switched to callerHandle. From my understanding tokenContext could be a class here in case of inline - that's the whole reason why we propagated CallerHandle.

MichalStrehovsky · 2024-03-13T06:34:25Z

funny enough, it made R2R'd corelib a bit smaller

It's also a small size improvement for native AOT: MichalStrehovsky/rt-sz#2 (comment)

jkotas · 2024-03-13T06:42:37Z

METHOD_BEING_COMPILED_CONTEXT should not be needed anymore and it can be deleted. Did you run into problems with deleting it?

EgorBo · 2024-03-13T13:22:07Z

METHOD_BEING_COMPILED_CONTEXT should not be needed anymore and it can be deleted. Did you run into problems with deleting it?

I found two places I don't have a good understanding what they're needed for:

runtime/src/coreclr/jit/inline.cpp

Line 1267 in d36beb7

rootContext->m_RuntimeContext = METHOD_BEING_COMPILED_CONTEXT();

- it sees to be used to detect recursive inlining or something like that
runtime/src/coreclr/vm/jitinterface.cpp

Line 5303 in d36beb7

pResult->contextHandle = METHOD_BEING_COMPILED_CONTEXT();

I might delete the 1st one but I think I'll need an SPMI collection for that so ideally after this PR is merged if you don't mind

# Conflicts: # src/coreclr/inc/jiteeversionguid.h # src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs

jkotas · 2024-03-13T14:28:56Z

I might delete the 1st one but I think I'll need an SPMI collection for that so ideally after this PR is merged if you don't mind

It is fine with me to do it in a follow up PR. Note that deleting METHOD_BEING_COMPILED_CONTEXT is JIT/EE interface breaking change, so you need to rev the GUID.

The second place is what enabled the limited shared generic inlining that should be superseded by your change. It checks for the happy path that the optimization was capable of handling.

EgorBo · 2024-03-13T14:30:34Z

/azp run runtime-coreclr crossgen2

azure-pipelines · 2024-03-13T14:30:46Z

Azure Pipelines successfully started running 1 pipeline(s).

EgorBo · 2024-03-13T14:34:52Z

The second place is what enabled the limited shared generic inlining that should be superseded by your change. It checks for the happy path that the optimization was capable of handling.

@jkotas do you mean it should be just pResult->contextHandle = callerHandleAsContext without those conditions, rights?

jkotas · 2024-03-13T14:43:10Z

It should be pResult->contextHandle = MAKE_CLASSCONTEXT(exactType.AsPtr()); that is there a few lines before the if check. The if check and pResult->contextHandle = METHOD_BEING_COMPILED_CONTEXT() should be deleted without replacement.

pResult->contextHandle = callerHandleAsContext without those conditions, rights?

This would not work well - it would be a code quality regression. The context needs to be exactType to carry as much instantiation details as possible into the inlinee.

EgorBo · 2024-03-13T16:00:58Z

/azp run runtime-coreclr crossgen2, runtime-coreclr outerloop

azure-pipelines · 2024-03-13T16:01:26Z

Azure Pipelines successfully started running 2 pipeline(s).

EgorBo · 2024-03-13T16:40:36Z

@jkotas anything else (assuming CI passes)?
I already have a couple of ideas for a follow up to clean up/improve CQ, so going to include METHOD_BEING_COMPILED_CONTEXT there

jkotas

Looks great! Thank you

EgorBo · 2024-03-14T04:17:59Z

Thanks for help!

EgorBo · 2024-03-18T04:56:26Z

Found at least 80 benchmarks improved by 10% or more: https://gist.github.com/EgorBo/b6424f7118ff176682f63875d89fb52e

EgorBo · 2024-03-19T12:43:12Z

Improvements on x64:

[Perf] Linux/x64: 114 Improvements on 3/14/2024 8:52:16 AM perf-autofiling-issues#31364
[Perf] Windows/x64: 94 Improvements on 3/14/2024 8:52:16 AM perf-autofiling-issues#31398
[Perf] Windows/x64: 73 Improvements on 3/14/2024 8:52:16 AM perf-autofiling-issues#31429
[Perf] Windows/x64: 91 Improvements on 3/14/2024 8:52:16 AM perf-autofiling-issues#31931
[Perf] Windows/x64: 16 Improvements on 3/11/2024 8:28:14 PM perf-autofiling-issues#31929

Improvements on arm64:

[Perf] Linux/arm64: 73 Improvements on 3/14/2024 8:52:16 AM perf-autofiling-issues#31629
[Perf] Windows/arm64: 55 Improvements on 3/14/2024 8:52:16 AM perf-autofiling-issues#31640
[Perf] Windows/arm64: 27 Improvements on 3/14/2024 8:52:16 AM perf-autofiling-issues#31623

Regressions on x64:

Regressions on arm64:

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 4, 2024

ghost assigned EgorBo Mar 4, 2024

jkotas reviewed Mar 4, 2024

View reviewed changes

Comment thread src/coreclr/inc/corinfo.h Outdated

build-analysis Bot mentioned this pull request Mar 5, 2024

Tracking issue for CI build timeouts #76454

Closed

Simpler change

5095668

EgorBo force-pushed the shared-generics-2 branch from d7e7396 to 5095668 Compare March 5, 2024 01:03

EgorBo marked this pull request as ready for review March 5, 2024 13:54

MihuBot mentioned this pull request Mar 5, 2024

[X64] [EgorBo] JIT: inline shared generics with runtime lookups inside MihuBot/runtime-utils#276

Open

jakobbotsch reviewed Mar 5, 2024

View reviewed changes

Comment thread src/coreclr/jit/importercalls.cpp Outdated

jakobbotsch reviewed Mar 5, 2024

View reviewed changes

Comment thread src/coreclr/jit/importer.cpp Outdated

Address feedback

4bc4269

jkotas reviewed Mar 6, 2024

View reviewed changes

Comment thread src/coreclr/vm/jitinterface.cpp Outdated

jakobbotsch reviewed Mar 6, 2024

View reviewed changes

Comment thread src/coreclr/jit/importer.cpp Outdated

jakobbotsch reviewed Mar 6, 2024

View reviewed changes

Comment thread src/coreclr/jit/inline.h Outdated

jakobbotsch reviewed Mar 6, 2024

View reviewed changes

Comment thread src/coreclr/jit/importercalls.cpp Outdated

EgorBo added 3 commits March 8, 2024 02:17

clean up

4f3353f

Address feedback

c4b9997

Clean up

0693f89

build-analysis Bot mentioned this pull request Mar 8, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

3 tasks

EgorBo added 3 commits March 8, 2024 05:09

fix build

f300235

clean up

6da6055

Class context?

f0da192

make formatter happy

10492e3

jakobbotsch approved these changes Mar 12, 2024

View reviewed changes

EgorBo mentioned this pull request Mar 12, 2024

Span<T> vs Span<String> difference in Morph: Structs/AddrExp #65815

Closed

MichalStrehovsky approved these changes Mar 13, 2024

View reviewed changes

jkotas reviewed Mar 13, 2024

View reviewed changes

Comment thread src/coreclr/vm/jitinterface.cpp Outdated

EgorBo added 3 commits March 13, 2024 14:25

use GetMethod

ad38ae1

Merge branch 'main' of github.com:dotnet/runtime into shared-generics-2

af79c6b

# Conflicts: # src/coreclr/inc/jiteeversionguid.h # src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs

Address feedback

09efb81

Address Jan's feedback

467cc06

jkotas approved these changes Mar 14, 2024

View reviewed changes

EgorBo merged commit 825f053 into dotnet:main Mar 14, 2024

EgorBo deleted the shared-generics-2 branch March 14, 2024 04:18

This was referenced Mar 14, 2024

SPMI: superpmi-collect broken #99758

Closed

Assertion failed '!"Unexpected tree op after call marked as tailcall"' #99798

Closed

JIT: Type fat pointer locals precisely, and avoid unnecessary normalization in inlining #99806

Merged

github-actions Bot locked and limited conversation to collaborators Apr 28, 2024

Conversation

EgorBo commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 4, 2024

Uh oh!

Uh oh!

EgorBo commented Mar 5, 2024

Uh oh!

azure-pipelines Bot commented Mar 5, 2024

Uh oh!

EgorBo commented Mar 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EgorBo commented Mar 9, 2024

Uh oh!

jkotas commented Mar 9, 2024

Uh oh!

EgorBo commented Mar 9, 2024

Uh oh!

EgorBo commented Mar 9, 2024

Uh oh!

jakobbotsch left a comment

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

EgorBo Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented Mar 13, 2024

Uh oh!

Uh oh!

jkotas commented Mar 13, 2024

Uh oh!

EgorBo commented Mar 13, 2024

Uh oh!

jkotas commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EgorBo commented Mar 13, 2024

Uh oh!

azure-pipelines Bot commented Mar 13, 2024

Uh oh!

EgorBo commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Mar 13, 2024

Uh oh!

EgorBo commented Mar 13, 2024

Uh oh!

azure-pipelines Bot commented Mar 13, 2024

Uh oh!

EgorBo commented Mar 13, 2024

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Mar 14, 2024

Uh oh!

EgorBo commented Mar 18, 2024

Uh oh!

EgorBo commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

EgorBo commented Mar 4, 2024 •

edited

Loading

jkotas commented Mar 13, 2024 •

edited

Loading

EgorBo commented Mar 13, 2024 •

edited

Loading

EgorBo commented Mar 19, 2024 •

edited

Loading