-
Notifications
You must be signed in to change notification settings - Fork 5.3k
JIT: Devirtualize non-shared generic virtual methods #122023
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables devirtualization support for generic virtual methods in the JIT compiler, allowing calls to generic virtual methods to be devirtualized when the exact type is known at JIT time. This optimization eliminates virtual dispatch overhead and enables further optimizations like inlining.
Key changes:
- Removes the assertion that previously blocked generic method devirtualization
- Generalizes array interface devirtualization to support generic virtual methods by renaming
wasArrayInterfaceDevirttoneedsMethodContext - Adds runtime lookup support for generic method instantiation parameters
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/jitinterface.cpp | Removes assertion blocking generic method devirtualization and adds logic to handle generic virtual methods using FindOrCreateAssociatedMethodDesc |
| src/coreclr/inc/corinfo.h | Renames wasArrayInterfaceDevirt to needsMethodContext to generalize the field meaning |
| src/coreclr/jit/jitconfigvalues.h | Adds JitEnableGenericVirtualDevirtualization configuration flag to control the feature |
| src/coreclr/jit/gentree.h | Adds IsGenericVirtual() helper method to identify generic virtual method calls |
| src/coreclr/jit/gentree.cpp | Updates IsDevirtualizationCandidate() to include generic virtual methods (non-AOT only) |
| src/coreclr/jit/importercalls.cpp | Implements devirtualization logic for generic virtual methods including runtime lookup handling, updates comments and variable names, and introduces DEVIRT label for control flow |
| src/coreclr/jit/inline.h | Renames arrayInterface field to needsMethodContext in InlineCandidateInfo struct |
| src/coreclr/jit/indirectcalltransformer.cpp | Updates to use renamed needsMethodContext field |
| src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs | Updates managed struct definition to match renamed field |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Updates to use renamed field in managed implementation |
| src/coreclr/tools/superpmi/superpmi-shared/agnostic.h | Updates SuperPMI data structure with renamed field |
| src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp | Updates SuperPMI recording/replay to use renamed field |
530e75d to
6e678a5
Compare
src/coreclr/jit/importercalls.cpp
Outdated
| // If we have a RUNTIMELOOKUP helper call for the method handle, | ||
| // we need to pass that as the inst param instead. | ||
| CallArg* const methHndArg = call->gtArgs.FindWellKnownArg(WellKnownArg::RuntimeMethodHandle); | ||
| GenTree* const methHndNode = methHndArg != nullptr ? methHndArg->GetEarlyNode() : nullptr; | ||
| if (methHndNode && methHndNode->OperIs(GT_RUNTIMELOOKUP)) | ||
| { | ||
| instParam = methHndNode; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this behavior?
It mixes syntax and semantics. Nothing guarantees that a runtime lookup appears as a GT_RUNTIMELOOKUP node. It could have been stored to a local and a number of other things.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a new commit including comments to clarify this behavior. Also updated the description of the PR.
src/coreclr/jit/importercalls.cpp
Outdated
| if (isGenericVirtual) | ||
| { | ||
| JITDUMP("Shared generic virtual devirt not supported yet\n"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The layering feels wrong to me. How can the VM side return an instantiating stub if the base method requires a runtime lookup? There is no way to create an instantiating stub at devirtualization time in that scenario.
I think it would make more sense if resolveVirtualMethod never returns an instantiation stub for GVMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it to VM so now the VM should never return an instantiating stub.
src/coreclr/vm/jitinterface.cpp
Outdated
| pDevirtMD = pDevirtMD->FindOrCreateAssociatedMethodDesc( | ||
| pDevirtMD, pExactMT, pExactMT->IsValueType() && !pDevirtMD->IsStatic(), pBaseMD->GetMethodInstantiation(), false); | ||
|
|
||
| // We still can't handle shared generic methods because we don't have | ||
| // the right generic context for runtime lookup. | ||
| // TODO: Remove this limitation. | ||
| if (pDevirtMD->IsInstantiatingStub()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly this will sometimes create odd instantiating stubs. For example, it may create an instantiating stub that instantiates over System.__Canon if the JIT got the method desc from a runtime lookup.
I do not think we need to create any instantiation stubs at all here. Should we instead pass allowInstParam = true to FindOrCreateAssociatedMethodDesc and then change this check to check if the returned function is shared?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also since the declaration takes BOOL I think it should be TRUE/FALSE instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, since we are now rejecting any shared generic methods, I think it's okay to pass allowInstParam = true.
But if we want to extend it to support shared generic methods as well, we will need to revisit it to allow it creating an instantiating stub again, and bail if it returns an instantiating stub over System.__Canon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in babad30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, since we are now rejecting any shared generic methods, I think it's okay to pass allowInstParam = true.
But if we want to extend it to support shared generic methods as well, we will need to revisit it to allow it creating an instantiating stub again, and bail if it returns an instantiating stub over System.__Canon.
I do not think that we should need to create instantiation stubs and then have the JIT remove them again. Instead we should make CORINFO_DEVIRTUALIZATION_INFO return the info necessary to let the JIT properly compute the generic context to pass. If the base method is a runtime lookup this will be some modified version of that runtime lookup. If the base method's instantiation is statically known then it will be some constant generic context (likely exactContext that already exists).
In either case I do not think we need to create an instantiating stub to then remove it again (and in the former case we can't even do that).
66f7a79 to
babad30
Compare
@MichalStrehovsky FYI I have a local branch that brings the non-shared GVM devirt support to R2R. NAOT still needs extra work in the JIT before it can be handled. I'll open another PR after this one merged, so that at least the changes to the managed type system can be reviewed first. |
jakobbotsch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from that. Thanks!
@davidwrighton can you take a look too?
|
@hez2010 sorry about the delay, I got stuck in a bunch of work just before the holidays and wasn't able to do the review before vacation began. |
|
I'm guessing the crossgen issues are related, I don't see them on other PRs. Can you see if you can reproduce them? |
I managed to reproduce the freebsd one on my local cross-build environment. Will investigate it soon to see whether it's related or not. |
|
We hit the assertion here: I think GVM applies the same with virtual stub, where in R2R mode, we might see GVM calls to non-virtuals. I think we should change the assertion to |
Seems reasonable to me. |
Enable devirtualization support for generic virtual methods.
When we see a base method having a method instantiation, we use
FindOrCreateAssociatedMethodDescto obtain the devirted method.Also introduced a jit knob so that it can be turned off at any time.
AOT support is not included in this PR, which needs additional work in managed type system.
Also, if we end up with an instantiating stub (i.e. a shared generic method that requires runtime lookup), we don't have the correct generic context so we need to bail out for now.
Codegen example:
Codegen diff:
Contributes to #112596
cc: @dotnet/jit-contrib