Support devirtualization for virtual methods that require an instantiating stub#128702
Support devirtualization for virtual methods that require an instantiating stub#128702hez2010 wants to merge 68 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Enables devirtualization of generic virtual methods and default interface methods (DIMs) on generic interfaces, by computing and passing the instantiation argument needed at runtime, instead of failing devirtualization in those cases.
Changes:
- In the VM JIT interface, remove the early bail-out for generic DIMs and shared generic virtual methods, and instead compute
instParamLookup/ re-resolve via the exact (non-shared) MethodDesc when possible; defer the instantiating-stub unwrap until aftertokenLookupContextis set. - In the managed JIT interface (
CorInfoImpl), add parallel logic to populateinstParamLookupfor generic DIM / generic virtual / array-interface devirtualization (R2R usesTypeDictionary/MethodHandlehelpers; NativeAOT path returnsFAILED_CANONfor now), and switchtokenLookupContextto a method context where appropriate. - In
DevirtualizationManager, drop the unconditional generic-virtual canon bail-out and, for R2R, only fail DIM resolution on variant dispatch (allowing generic DIMs); keep the stricter pre-existing behavior for NativeAOT.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/coreclr/vm/jitinterface.cpp | Allow devirt to generic DIM / shared GVM by computing exact MD and instParamLookup; move instantiating-stub unwrap after context setup. |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Add instParamLookup population for generic DIM / GVM / array-interface cases; adjust tokenLookupContext; gate AOT vs R2R behavior. |
| src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs | For R2R, only fail DIM resolution on variant dispatch; remove unconditional shared-GVM canon bail-out. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs | Update stale comment numbering after removing the DIM-restriction note. |
|
@MihuBot -nuget |
davidwrighton
left a comment
There was a problem hiding this comment.
LGTM, but I'd like @MichalStrehovsky to sign off on the native aot test issues being unrelated before we merge.
|
@MichalStrehovsky I disabled the generic recursion test for NativeAOT in 87b4e0c due to #129855. Can you take a look? |
These are not scenarios we're concerned about for native AOT, correct? Otherwise it would be nice to write it in a way that it doesn't trigger a recursion. |
The test here performs a runtime lookup through delegates to resolve to itself with a different instantiation to exercise whether we use the right generic context before / after devirtualization against the same method, and that's why a generic recursion is here. |
|
I added several tests to cover NativeAOT's behavior of unboxing stubs + runtime lookups in a non-recursive way. |
|
/azp run runtime-coreclr outerloop, runtime-coreclr pgo, runtime-coreclr pgostress, runtime-nativeaot-outerloop |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
I updated the branch to resolve this. Also fixed a copy-paste error which dismissed the approval. @MichalStrehovsky Can you trigger the run again? |
Enable devirtualization of virtual methods that require an instantiating stub.
We have landed the refactor in the JIT so that now we are able to handle instantiating stub naturally without a JIT change.
This covers both shared generic virtual methods that don't require a runtime lookup, and generic interface methods that have a default implementation.
GVM devirt for NativeAOT requires more changes which is out of scope of this PR.
Before:
After:
Similar for R2R:
For NativeAOT:
Before:
After:
Also added a couple of tests from #43668.
Closes #9588
Contributes to #112596
/cc: @jakobbotsch @davidwrighton @MichalStrehovsky