Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

JIT: inline shared generics with runtime lookups inside#99265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
EgorBo merged 45 commits intodotnet:mainfromEgorBo:shared-generics-2
Mar 14, 2024

Conversation

EgorBo
Copy link
Member

@EgorBoEgorBo commentedMar 4, 2024•
edited
Loading

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

Based on#81635.

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

[MethodImpl(MethodImplOptions.NoInlining)]boolTest<T>()=>Callee<T>();boolCallee<T>()=>typeof(T).IsValueType;

Codegen forTest<string> in Main:

; Assembly listing for method Program:Test[System.__Canon]():bool:thispushrbxsubrsp,48mov      qword ptr[rsp+0x28],rdxmovrbx,rcxmovrcx, qword ptr[rdx+0x10]movrax, qword ptr[rcx+0x10]testrax,raxje       SHORT G_M000_IG04movrdx,raxjmp      SHORT G_M000_IG05G_M000_IG04:movrcx,rdxmovrdx,0x7FFCC5FE7088call     CORINFO_HELP_RUNTIMEHANDLE_METHODmovrdx,raxG_M000_IG05:movrcx,rbxcall[Program:Callee[System.__Canon]():bool:this]   ;;;  <---- not inlinednopaddrsp,48poprbxret; Total bytes of code 68

Codegen forTest<string> in this PR:

; Assembly listing for method Program:Test[System.__Canon]():ubyte:thisxoreax,eaxret

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.

redknightlois reacted with hooray emojiam11, hez2010, En3Tho, punchready, neon-sunset, hypeartist, ShreyasJejurkar, and PaulusParssinen reacted with rocket emoji
@ghostghost added the area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labelMar 4, 2024
@ghostghost assignedEgorBoMar 4, 2024
@ghost
Copy link

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

Issue Details

Closes#81432

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

[MethodImpl(MethodImplOptions.NoInlining)]boolTest<T>()=>Callee<T>();boolCallee<T>()=>typeof(T).IsValueType;

Codegen forTest<string> in Main:

; Assembly listing for method Program:Test[System.__Canon]():bool:thispushrbxsubrsp,48mov      qword ptr[rsp+0x28],rdxmovrbx,rcxmovrcx, qword ptr[rdx+0x10]movrax, qword ptr[rcx+0x10]testrax,raxje       SHORT G_M000_IG04movrdx,raxjmp      SHORT G_M000_IG05G_M000_IG04:movrcx,rdxmovrdx,0x7FFCC5FE7088call     CORINFO_HELP_RUNTIMEHANDLE_METHODmovrdx,raxG_M000_IG05:movrcx,rbxcall[Program:Callee[System.__Canon]():bool:this]   ;;;  <---- not inlinednopaddrsp,48poprbxret; Total bytes of code 68

Codegen forTest<string> in this PR:

; Assembly listing for method Program:Test[System.__Canon]():ubyte:thisxoreax,eaxret

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
Copy link
MemberAuthor

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

@azure-pipelinesAzure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBoEgorBo marked this pull request as ready for reviewMarch 5, 2024 13:54
@EgorBo
Copy link
MemberAuthor

@MihuBot

@EgorBo
Copy link
MemberAuthor

@jkotas one thing I struggle with currently is when I havetokenContext being class (CORINFO_CONTEXTFLAGS_CLASS) -- how do I know whether to useCORINFO_LOOKUP_CLASSPARAM orCORINFO_LOOKUP_THISOBJ ?

@jkotas
Copy link
Member

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 fromCORINFO_CONTEXTFLAGS_CLASS context alone. You need to have the current method to figure it out.

@EgorBo
Copy link
MemberAuthor

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 fromCORINFO_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 them_methodBeingCompiled (which is the root method). So I presume I need to extend the JIT-EE interface

@EgorBo
Copy link
MemberAuthor

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

Copy link
Member

@jakobbotschjakobbotsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

JIT side LGTM.

EgorBo reacted with heart emoji
Copy link
Member

@MichalStrehovskyMichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@@ -827,7 +827,7 @@ public void CompileMethod(MethodWithGCInfo methodCodeNodeNeedingCode, Logger log
}
}

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm curious whycallerHandle can be unused here (we usepResolvedToken.tokenContext to get the context instead), butembedGenericHandle andgetCallInfo need thecallerHandle and don't use context. Since the tests are passing, maybecallerHandle is redundant withtokenContext?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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
Copy link
Member

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)

EgorBo reacted with thumbs up emoji

@jkotas
Copy link
Member

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

@EgorBo
Copy link
MemberAuthor

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:

  1. rootContext->m_RuntimeContext =METHOD_BEING_COMPILED_CONTEXT();
    - it sees to be used to detect recursive inlining or something like that
  2. 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
Copy link
Member

jkotas commentedMar 13, 2024•
edited
Loading

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 deletingMETHOD_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 reacted with thumbs up emoji

@EgorBo
Copy link
MemberAuthor

/azp run runtime-coreclr crossgen2

@azure-pipelinesAzure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo
Copy link
MemberAuthor

EgorBo commentedMar 13, 2024•
edited
Loading

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 justpResult->contextHandle = callerHandleAsContext without those conditions, rights?

@jkotas
Copy link
Member

It should bepResult->contextHandle = MAKE_CLASSCONTEXT(exactType.AsPtr()); that is there a few lines before the if check. The if check andpResult->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 beexactType to carry as much instantiation details as possible into the inlinee.

EgorBo reacted with thumbs up emoji

@EgorBo
Copy link
MemberAuthor

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

@azure-pipelinesAzure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBo
Copy link
MemberAuthor

@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 includeMETHOD_BEING_COMPILED_CONTEXT there

Copy link
Member

@jkotasjkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks great! Thank you

@EgorBoEgorBo merged commit825f053 intodotnet:mainMar 14, 2024
@EgorBo
Copy link
MemberAuthor

Thanks for help!

AndyAyersMS reacted with hooray emoji

@EgorBo
Copy link
MemberAuthor

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

jkotas, En3Tho, and PaulusParssinen reacted with hooray emoji

@EgorBo
Copy link
MemberAuthor

EgorBo commentedMar 19, 2024•
edited
Loading

Improvements on x64:

Improvements on arm64:

Regressions on x64:

Regressions on arm64:

saul, PaulusParssinen, and Giorgi reacted with rocket emoji

@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsApr 28, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@jkotasjkotasjkotas approved these changes

@jakobbotschjakobbotschjakobbotsch approved these changes

@MichalStrehovskyMichalStrehovskyMichalStrehovsky approved these changes

Assignees

@EgorBoEgorBo

Labels
area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
4 participants
@EgorBo@jkotas@MichalStrehovsky@jakobbotsch
[8]ページ先頭
©2009-2025 Movatter.jp