JIT: Unify to instParamLookup and improve GVM test coverage#126947
JIT: Unify to instParamLookup and improve GVM test coverage#126947hez2010 wants to merge 2 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Refactors CoreCLR devirtualization metadata to use a unified instParamLookup and expands JIT test coverage for generic virtual method (GVM) scenarios to support follow-on GVM devirtualization work.
Changes:
- Add a new GVM-focused runtime lookup/delegate test to broaden devirtualization coverage.
- Replace
CORINFO_DEVIRTUALIZATION_INFOad-hoc flags withinstParamLookup, and update JIT devirtualization logic to consume it. - Update SuperPMI and the managed JitInterface type definitions/serialization to match the new
CORINFO_DEVIRTUALIZATION_INFOshape (plus bump the JIT/EE versioning GUID).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/Generics/VirtualMethods/generic_virtual_methods.cs | Adds additional GVM/runtime-lookup-related test coverage. |
| src/coreclr/vm/jitinterface.cpp | Initializes/produces instParamLookup for devirtualization results (replacing older flags). |
| src/coreclr/jit/importercalls.cpp | Plumbs instParamLookup through devirtualization + guarded devirtualization candidate tracking and inserts instantiation args when required. |
| src/coreclr/jit/morph.cpp | Simplifies lookup-tree helpers to take CORINFO_LOOKUP directly. |
| src/coreclr/jit/compiler.h | Updates declarations for the new lookup-tree helper signatures and GDV candidate plumbing. |
| src/coreclr/inc/corinfo.h | Updates CORINFO_DEVIRTUALIZATION_INFO to carry instParamLookup instead of legacy flags; documents “no lookup needed” sentinel. |
| src/coreclr/inc/jiteeversionguid.h | Bumps the JIT/EE version GUID for the interface change. |
| src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs | Mirrors the CORINFO_DEVIRTUALIZATION_INFO layout change in managed definitions. |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Updates managed resolveVirtualMethod implementation to populate instParamLookup and method-context behavior. |
| src/coreclr/tools/superpmi/superpmi-shared/agnostic.h | Updates SuperPMI record structs to include instParamLookup. |
| src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp | Records/restores/dumps instParamLookup in SuperPMI method contexts. |
| @@ -8601,8 +8601,9 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) | |||
| info->detail = CORINFO_DEVIRTUALIZATION_UNKNOWN; | |||
| memset(&info->resolvedTokenDevirtualizedMethod, 0, sizeof(info->resolvedTokenDevirtualizedMethod)); | |||
| memset(&info->resolvedTokenDevirtualizedUnboxedMethod, 0, sizeof(info->resolvedTokenDevirtualizedUnboxedMethod)); | |||
There was a problem hiding this comment.
info->instParamLookup.lookupKind.runtimeLookupKind is left uninitialized here (only needsRuntimeLookup is set). Even when needsRuntimeLookup is false, this can leak uninitialized data into diagnostics/SuperPMI recordings and make results non-deterministic. Initialize the whole instParamLookup (e.g., zero it) or explicitly set runtimeLookupKind to a defined value before returning.
| memset(&info->resolvedTokenDevirtualizedUnboxedMethod, 0, sizeof(info->resolvedTokenDevirtualizedUnboxedMethod)); | |
| memset(&info->resolvedTokenDevirtualizedUnboxedMethod, 0, sizeof(info->resolvedTokenDevirtualizedUnboxedMethod)); | |
| memset(&info->instParamLookup, 0, sizeof(info->instParamLookup)); |
| GenTree* instParam = getLookupTree(&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 produce the lookup for devirtualized call, sorry.\n"); | ||
| return; | ||
| } | ||
|
|
||
| call->gtArgs.InsertInstParam(this, instParam); |
There was a problem hiding this comment.
The local name instParam is reused for two different concepts in the same method (CORINFO_METHOD_HANDLE instParam earlier, then GenTree* instParam here). This shadowing is easy to misread and can lead to mistakes during future edits. Rename one/both (e.g., instParamHnd vs instParamNode) to keep the handle and tree clearly distinct.
| GenTree* instParam = getLookupTree(&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 produce the lookup for devirtualized call, sorry.\n"); | |
| return; | |
| } | |
| call->gtArgs.InsertInstParam(this, instParam); | |
| GenTree* instParamNode = getLookupTree(&dvInfo.instParamLookup, GTF_ICON_METHOD_HDL, compileTimeHandle); | |
| if (instParamNode == nullptr) | |
| { | |
| // If we're inlining, impLookupToTree can return nullptr after recording a fatal observation. | |
| JITDUMP("Failed to produce the lookup for devirtualized call, sorry.\n"); | |
| return; | |
| } | |
| call->gtArgs.InsertInstParam(this, instParamNode); |
|
Test failure seems unrelated. |
Extracted from #123323. This PR is the first step in splitting the shared GVM devirtualization work.
It does two things:
CORINFO_DEVIRTUALIZATION_INFOwith a singleinstParamLookupContributes to #112596
/cc: @jakobbotsch