Uh oh!
There was an error while loading. Please reload this page.
Support devirtualization for virtual methods that require runtime lookups - #129806
Support devirtualization for virtual methods that require runtime lookups#129806hez2010 wants to merge 23 commits into
Conversation
hez2010
commented
Jun 24, 2026
@MihuBot -nuget |
hez2010
commented
Jun 25, 2026
@MihuBot -nuget -dependsOn 128702 |
5497d14 to
309d8fdCompareUh oh!
There was an error while loading. Please reload this page.
| CORINFO_METHOD_HANDLE methodHnd; | ||
| CORINFO_CONTEXT_HANDLE exactContextHnd; | ||
| ILLocation ilLocation; | ||
| CORINFO_RESOLVED_TOKEN resolvedToken; |
There was a problem hiding this comment.
Resolved tokens are large structures. Does this show up on memory use?
There was a problem hiding this comment.
I didn't measure it yet, but it is necessary for computing the runtime lookup. I can't think of a better way than disabling late devirtualization for this if we don't want to store a resolved token.
| // If we don't have the right context, try recover it from the tree. | ||
| // | ||
| if (!isCurrentContext) | ||
| { | ||
| CallArg* runtimeMethodHandleArg = nullptr; | ||
| if ((call->gtControlExpr != nullptr) && call->gtControlExpr->OperIs(GT_CALL)) | ||
| { | ||
| runtimeMethodHandleArg = | ||
| call->gtControlExpr->AsCall()->gtArgs.FindWellKnownArg(WellKnownArg::RuntimeMethodHandle); | ||
| } | ||
| if (runtimeMethodHandleArg != nullptr && runtimeMethodHandleArg->GetNode()->OperIs(GT_RUNTIMELOOKUP)) | ||
| { | ||
| if (runtimeMethodHandleArg->GetNode()->AsRuntimeLookup()->Lookup()->OperIs(GT_CALL)) | ||
| { | ||
| GenTreeCall* const helperCall = | ||
| runtimeMethodHandleArg->GetNode()->AsRuntimeLookup()->Lookup()->AsCall(); | ||
| if (helperCall->IsHelperCall(CORINFO_HELP_RUNTIMEHANDLE_METHOD) || | ||
| helperCall->IsHelperCall(CORINFO_HELP_RUNTIMEHANDLE_CLASS)) | ||
| { | ||
| runtimeLookupContext = helperCall->gtArgs.GetArgByIndex(0)->GetNode(); | ||
| } | ||
| } | ||
| } | ||
| if (runtimeLookupContext == nullptr) | ||
| { | ||
| JITDUMP("Late devirt needs a runtime lookup context cannot be figured out. Bail out.\n"); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why can this not be done where it is needed?
Trying to smuggle IR through across phases without actually adding it does not scale well. I do not think this is a workable way of doing this.
There was a problem hiding this comment.
Ah, dcInfo is not the info we save. Ok, then it looks better.
There was a problem hiding this comment.
Trying to smuggle IR through across phases without actually adding it does not scale well.
Alternatively we will need to add something to GenTreeCall to carry the original lookup context tree.
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "a9780b4c86ad8a2f839ef4abad70f63e3737b6e3",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "de625a927aa4e6f70cda5eaf247cf54c225d158a",
"last_reviewed_commit": "a9780b4c86ad8a2f839ef4abad70f63e3737b6e3",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "de625a927aa4e6f70cda5eaf247cf54c225d158a",
"last_recorded_worker_run_id": "29681945714",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "a9780b4c86ad8a2f839ef4abad70f63e3737b6e3",
"review_id": 4730548930
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: The problem is real and worthwhile. Today the JIT bails out of devirtualizing a shared generic virtual method whose instantiation argument requires a runtime lookup (CORINFO_DEVIRTUALIZATION_FAILED_CANON). This PR lets such calls devirtualize (still paying the runtime lookup) so the callee can be inlined and benefit from escape analysis, struct promotion, etc. The demonstrated codegen improvement is significant and it contributes to #112596.
Approach: Reasonable and consistent with existing infrastructure. It adds a new DevirtualizedMethodDescSlot dictionary entry kind wired through the VM, the R2R/NativeAOT ComputeRuntimeLookupForSharedGenericToken, and SuperPMI, threads a callerMethod input into resolveVirtualMethod, and propagates the runtime-lookup context tree from dcInfo into getLookupTree/getRuntimeLookupTree. The JIT-EE GUID is correctly bumped. NativeAOT is intentionally left as a TODO. The trickiest part is late devirtualization recovering the correct generic context from the existing call IR; the author added a LateDevirtualizationInfo.resolvedToken and targeted tests for the immediate/late/late-root context cases.
Summary: CORINFO_DEVIRTUALIZATION_INFO construction sites now initialize callerMethod, and the shared-MT vs shared-method cases bail vs. runtime-lookup as intended). However, two areas warrant maintainer judgment: (1) the impRuntimeLookupToTree behavioral change is broader than the devirt feature and touches all optimized testForNull runtime lookups (see inline); and (2) jakobbotsch already raised concerns about smuggling the lookup-context IR across phases via dcInfo and about the memory cost of storing a full CORINFO_RESOLVED_TOKEN in LateDevirtualizationInfo. These are design/CQ tradeoffs best confirmed by the JIT area owners plus SPMI diffs, not blockers I can resolve from the diff alone.
Detailed Findings
⚠️ Broadened behavior in impRuntimeLookupToTree — un-spilled helper call in optimized code
The opts.OptimizationEnabled() branch now returns the raw helper GT_CALL instead of always spilling to a temp. This applies to every testForNull runtime lookup in optimized code, well beyond the devirtualization scenario the PR targets. Flagged inline on src/coreclr/jit/importer.cpp; please validate with a full SPMI/CQ run that no unrelated regressions or duplicated side-effecting calls result, and consider scoping it.
✅ callerMethod initialization is complete
The new required CORINFO_DEVIRTUALIZATION_INFO.callerMethod input is set at all three JIT construction sites (importercalls.cpp lines 8097, 8183, 9296) from call->gtInlineContext->GetCallee(). An earlier Copilot comment worried about uninitialized garbage in the GDV exact-classes loop; that concern does not apply to the current head — line 8097 initializes it. The VM/R2R runtime-lookup paths that dereference callerMethod are only reached after pResolvedTokenVirtualMethod != nullptr, and in the JIT that token is only non-null on the direct-devirt path (GDV likely/exact still pass nullptr), so GetMethod/HandleToObject on callerMethod is not exercised with an unset handle today. Worth a maintainer double-check that no external JIT-EE consumer leaves callerMethod unset.
✅ Shared-MT vs shared-method distinction
The VM (resolveVirtualMethodHelper) and managed (CorInfoImpl.cs) paths correctly bail with FAILED_CANON when a shared MethodTable instantiation is required (cannot be recovered even as a runtime lookup) and only take the new DevirtualizedMethodDescSlot runtime-lookup path when the method instantiation is canonical. The isUnboxingStubOfInstantiatingStub case was split accordingly, and the unboxed-entry lookup now unwraps an instantiating stub (jitinterface.cpp 8979).
✅ Late-devirtualization context recovery + tests
The RuntimeLookupInlining test exercises immediate inlining, late devirtualization of an inlinee call (context mismatch requiring recovery of the runtime-lookup context from the RuntimeMethodHandle arg), and the late-root case. The bail-out when the context tree cannot be recovered (impDevirtualizeCall) is a safe conservative fallback. This addresses the JIT guidance's expectation of targeted tests for an observable behavior change.
💡 Style nit in jitinterface.cpp
The new single-line braceless if at line 8979 is inconsistent with the file's bracing style; flagged inline (non-blocking).
💡 Open design questions from maintainer review
jakobbotsch questioned passing the lookup context IR through dcInfo across phases and the memory footprint of embedding a full CORINFO_RESOLVED_TOKEN in LateDevirtualizationInfo (resolved tokens are large). The author acknowledged both and noted the alternative would be adding a field to GenTreeCall. These remain unresolved design tradeoffs for the area owners to sign off on.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 190.1 AIC · ⌖ 11.5 AIC · ⊞ 10K
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Add support for devirtualizing virtual methods that require a runtime lookup. In this case we can't avoid the expensive runtime lookup, but at least the callee now can be devirtualized and inlined, so that they can benefit from the downstream optimizations like escape analysis, struct promotion and etc.
When we see an inexact instantiating stub on an exact class, we compute a runtime lookup against the shared generic token of the virtual method.
Codegen for
Call:Before:
After:
Contributes to #112596