JIT: Devirtualize shared generic virtual methods#123323
JIT: Devirtualize shared generic virtual methods#123323hez2010 wants to merge 16 commits intodotnet:mainfrom
Conversation
github-actions
bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib |
There was a problem hiding this comment.
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. |
| { | ||
| info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CANON; | ||
| return false; | ||
| pDevirtMD = MethodDesc::FindOrCreateAssociatedMethodDesc( |
There was a problem hiding this comment.
This part may be a bit confusing.
Ideally, we should let devirtualizedMethod be the canonical method, and return the instantiating stub through exactContext.
But the existing array interface devirt is doing:
- return the instantiating stub through
devirtualizedMethod - call
getInstantiatedEntryfrom JIT to get both the entrypoint and the instantiating stub - insert the instantiating stub as an inst param
So here I chose to return the instantiating stub through devirtualizedMethod as well so that I can share the same logic with the array interface devirtualization. I think the refactor around this place can be a follow-up PR?
hez2010
changed the title
hez2010
force-pushed
the
gvm-devirt-shared
branch
from
bf5b09a to
c8b7f69
Compare
hez2010
force-pushed
the
gvm-devirt-shared
branch
from
15a3eee to
eb692c5
Compare
| if (dvInfo.instParamLookup.constLookup.accessType != IAT_VALUE) | ||
| { | ||
| JITDUMP("Unsupported devirt instantiation lookup access type %d\n", | ||
| dvInfo.instParamLookup.constLookup.accessType); | ||
| return; | ||
| } |
There was a problem hiding this comment.
impDevirtualizeCall treats IAT_PVALUE as a 'const inst param' in impDevirtualizedCallHasConstInstParam, but then immediately bails out if accessType != IAT_VALUE. Some hosts (e.g., crossgen2’s CreateConstLookupToSymbol) can legitimately produce IAT_PVALUE const lookups, so this will block devirtualization in those cases. Either handle IAT_PVALUE here (and avoid extracting a handle directly), or tighten impDevirtualizedCallHasConstInstParam to only return true for IAT_VALUE so the control flow is consistent.
| // Insert the instantiation argument when necessary. | ||
| if (needsInstParam) | ||
| { | ||
| assert(call->gtArgs.FindWellKnownArg(WellKnownArg::InstParam) == nullptr); | ||
|
|
||
| CORINFO_METHOD_HANDLE compileTimeHandle = derivedMethod; | ||
| if (needsCompileTimeLookup) | ||
| { | ||
| compileTimeHandle = instantiatingStub; | ||
| } | ||
|
|
||
| CORINFO_RESOLVED_TOKEN* lookupToken = | ||
| (pDerivedResolvedToken->tokenScope != nullptr) ? pDerivedResolvedToken : pResolvedToken; | ||
| GenTree* instParam = | ||
| impLookupToTree(lookupToken, &dvInfo.instParamLookup, GTF_ICON_METHOD_HDL, compileTimeHandle); | ||
|
|
||
| if (instParam == nullptr) | ||
| { | ||
| // If we're inlining, impLookupToTree can return nullptr after recording a fatal observation. | ||
| JITDUMP("Failed to materialize instantiation argument for devirtualized call, sorry.\n"); | ||
| return; | ||
| } | ||
|
|
||
| call->gtArgs.InsertInstParam(this, instParam); | ||
| } |
There was a problem hiding this comment.
The PR enables devirtualization of shared generic virtual method targets by introducing instParamLookup and new runtime lookup kinds. There are many existing JIT tests for generic virtual methods/devirtualization, but none updated here to cover the new shared-target + instantiation-arg lookup behavior (including late devirtualization). Please add/extend a JIT regression test that exercises this scenario (ideally with late devirt enabled) to guard against future regressions in instParamLookup materialization and the new DevirtualizedMethodDescSlot runtime lookup encoding.
| ((dvInfo.instParamLookup.constLookup.accessType == IAT_VALUE && | ||
| dvInfo.instParamLookup.constLookup.handle != nullptr) || | ||
| (dvInfo.instParamLookup.constLookup.accessType == IAT_PVALUE && | ||
| dvInfo.instParamLookup.constLookup.addr != nullptr)); |
There was a problem hiding this comment.
impDevirtualizedCallHasConstInstParam treats IAT_PVALUE lookups as valid, but impDevirtualizeCall later bails out unless accessType == IAT_VALUE. This inconsistency can silently disable inst-param insertion for const lookups represented as indirections (e.g., symbol-based handles). Either restrict the helper to IAT_VALUE-only or extend impDevirtualizeCall to also handle IAT_PVALUE (using constLookup.addr).
| ((dvInfo.instParamLookup.constLookup.accessType == IAT_VALUE && | |
| dvInfo.instParamLookup.constLookup.handle != nullptr) || | |
| (dvInfo.instParamLookup.constLookup.accessType == IAT_PVALUE && | |
| dvInfo.instParamLookup.constLookup.addr != nullptr)); | |
| (dvInfo.instParamLookup.constLookup.accessType == IAT_VALUE && | |
| dvInfo.instParamLookup.constLookup.handle != nullptr); |
| } | ||
|
|
||
| CORINFO_RESOLVED_TOKEN* lookupToken = | ||
| (pDerivedResolvedToken->tokenScope != nullptr) ? pDerivedResolvedToken : pResolvedToken; |
There was a problem hiding this comment.
instParam insertion chooses lookupToken as (resolvedTokenDevirtualizedMethod.tokenScope != nullptr) ? ... : pResolvedToken, but pResolvedToken is allowed to be nullptr in some callers (e.g., indirectcalltransformer). If needsInstParam is true and tokenScope is null (common for non-R2R), this will dereference a null pointer. Add a null check and bail out (or ensure callers always supply a resolved token when instParamLookup may be used).
| (pDerivedResolvedToken->tokenScope != nullptr) ? pDerivedResolvedToken : pResolvedToken; | |
| (pDerivedResolvedToken->tokenScope != nullptr) ? pDerivedResolvedToken : pResolvedToken; | |
| if (lookupToken == nullptr) | |
| { | |
| JITDUMP("Failed to materialize instantiation argument for devirtualized call: no resolved token.\n"); | |
| return; | |
| } |
There was a problem hiding this comment.
GVMs are not being profiled yet so they won't hit the GDV code path.
|
All pri0+pri1 tests except the interop known failure are passing. Mark it as ready for review now. Details |
hez2010
force-pushed
the
gvm-devirt-shared
branch
from
f22739f to
21623ad
Compare
| if (dvInfo.instParamLookup.constLookup.accessType != IAT_VALUE) | ||
| { | ||
| JITDUMP("Unsupported devirt instantiation lookup access type %d\n", | ||
| dvInfo.instParamLookup.constLookup.accessType); | ||
| return; | ||
| } | ||
|
|
||
| // We don't expect R2R to end up here, since it does not (yet) support | ||
| // array interface devirtualization. | ||
| // | ||
| assert(!IsAot()); | ||
| instantiatingStub = (CORINFO_METHOD_HANDLE)dvInfo.instParamLookup.constLookup.handle; |
There was a problem hiding this comment.
impDevirtualizeCall treats any constant instantiation argument as requiring instParamLookup.constLookup.accessType == IAT_VALUE and then casts constLookup.handle to a method handle. However, AOT hosts can legally return constant lookups as IAT_PVALUE (e.g., an indirection cell; see CreateConstLookupToSymbol setting IAT_PVALUE), which makes impDevirtualizedCallHasConstInstParam return true but then this code bails out and prevents devirtualization (and would not be able to obtain instantiatingStub from handle anyway). Please handle IAT_PVALUE here (or narrow impDevirtualizedCallHasConstInstParam to only treat IAT_VALUE as a supported const inst-param shape) so NativeAOT/R2R const lookups don’t regress devirtualization.
2 participants
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