-
Notifications
You must be signed in to change notification settings - Fork 5.3k
JIT: Devirtualize shared generic virtual methods #123323
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 JIT devirtualization for shared generic virtual methods (GVM) that don't require runtime lookups. Previously, all shared GVMs were blocked from devirtualization due to concerns about having the right generic context. This change unblocks devirtualization when the instantiating stub doesn't need a runtime lookup, by checking for the presence of a GT_RUNTIMELOOKUP node before proceeding.
Changes:
- Introduced
needsMethodContextflag to track when a method context is needed for devirtualization - For shared generic methods, obtain the instantiating stub and set
needsMethodContext = true - Unified handling of array interface and generic virtual method devirtualization paths
- Added runtime lookup check in the JIT to bail out when a lookup is needed but context is unavailable
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/vm/jitinterface.cpp | Added logic to detect shared generic methods and obtain instantiating stubs, unified array and GVM devirtualization handling |
| src/coreclr/jit/importercalls.cpp | Updated assertions to allow GVM in AOT scenarios, added runtime lookup check to prevent devirtualization when context is unavailable |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Failures seem to be caused by missing context during spmi replay. Otherwise all tests are passing. |
bf5b09a to
c8b7f69
Compare
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
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
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
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 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.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
e48f6a7 to
583277b
Compare
|
I realised that the existing runtime lookup tests are not actually doing runtime lookup, they are const lookup. |
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
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
| // If we don't know the array type exactly we may have the wrong interface type here. | ||
| // Bail out. | ||
| // | ||
| if (!isExact) |
Copilot
AI
Feb 10, 2026
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 needsCompileTimeLookup path unconditionally bails out when !isExact with an "Array interface devirt" message, but needsCompileTimeLookup can now also be true for shared generic virtual method devirtualization (not just array-interface devirt). This can prevent devirtualization in cases where objClassIsFinal/derivedMethodIsFinal would otherwise allow it. Consider gating the !isExact bailout (and the diagnostic text) to the array-interface scenario only, and allow the GVM path to proceed without requiring isExact.
| // If we don't know the array type exactly we may have the wrong interface type here. | |
| // Bail out. | |
| // | |
| if (!isExact) | |
| // For array interface devirtualization, if we don't know the array type exactly | |
| // we may have the wrong interface type here. Bail out. | |
| // | |
| if (!call->IsGenericVirtual(this) && !isExact) |
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
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
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
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
| impl.IsCanonicalMethod(CanonicalFormKind.Specific)) | ||
| { | ||
| #if READYTORUN | ||
| MethodWithToken originalImplWithToken = new MethodWithToken(originalImpl, resolver.GetModuleTokenForMethod(originalImpl.GetTypicalMethodDefinition(), allowDynamicallyCreatedReference: false, throwIfNotFound: false), null, false, null, null); |
Copilot
AI
Feb 11, 2026
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.
In the const instParamLookup path, originalImplWithToken is created with GetModuleTokenForMethod(... throwIfNotFound: false) but the returned ModuleToken isn’t checked for IsNull before constructing MethodWithToken / creating the ReadyToRunHelperId.MethodHandle node. If the method isn’t representable in the R2R image, this can lead to asserts/NREs rather than gracefully failing devirtualization. Consider using throwIfNotFound: true here, or checking IsNull and returning CORINFO_DEVIRTUALIZATION_FAILED_DECL_NOT_REPRESENTABLE (or the appropriate detail) before building the helper node.
| MethodWithToken originalImplWithToken = new MethodWithToken(originalImpl, resolver.GetModuleTokenForMethod(originalImpl.GetTypicalMethodDefinition(), allowDynamicallyCreatedReference: false, throwIfNotFound: false), null, false, null, null); | |
| var originalImplModuleToken = resolver.GetModuleTokenForMethod(originalImpl.GetTypicalMethodDefinition(), allowDynamicallyCreatedReference: false, throwIfNotFound: false); | |
| if (originalImplModuleToken.IsNull) | |
| { | |
| info->detail = CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_FAILED_DECL_NOT_REPRESENTABLE; | |
| return false; | |
| } | |
| MethodWithToken originalImplWithToken = new MethodWithToken(originalImpl, originalImplModuleToken, null, false, null, null); |
|
Test failures seem unrelated. |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Previously in #122023, we hit an issue with GVM devirtualization when the devirtualized target is a shared generic method. GVM calls are imported with a runtime lookup that is specific to the base method. After devirtualization, the call requires the instantiation argument for the implementing method, and the existing lookup cannot be reused.
This PR unblocks devirtualization for shared generic targets by ensuring the call receives the correct instantiation parameter for the devirtualized method:
The multiple ad-hoc flags in
dvInfonow have been unified into a singleinstParamLookupWhen the target does not require a runtime lookup, we already know the exact generic context. We pass the instantiating stub as the inst param (shared with the existing array interface devirtualization path).
When the target requires a runtime lookup, we now introduced a new
DictionaryEntryKind::DevirtualizedMethodDescSlot, and pass it to theinstParamLookupso that later the VM knows that it needs to encode the class token from the devirtualized method instead of the original token.To make this work for late devirtualization as well which runs as a post-import phase,
impTokenToHandlehas been changed so that after importation completes, runtime lookup trees are built using a way that doesn't append statements.Also due to the
instParamLookupchange I implement the support for R2R as well.Example:
Codegen diff:
Contributes to #112596