[R2R] Expand discovery of non-generic virtual methods on generic types#128652
[R2R] Expand discovery of non-generic virtual methods on generic types#128652BrzVlad wants to merge 2 commits into
Conversation
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR extends crossgen2/ReadyToRun virtual-dispatch dependency discovery so that non-generic virtual method slots on generic type instantiations can trigger compilation of the correct concrete implementations (in addition to the existing GVM (generic virtual method) mechanism).
Changes:
- Introduces a new
VirtualMethodUseNodemarker and a corresponding cache onNodeFactory. - Marks non-GVM virtual slots as “used” from
MethodFixupSignatureforVirtualEntryfixups. - Adds conditional static dependencies to
InheritedVirtualMethodsNodeto compile per-type implementations when a corresponding slot is marked used, and broadens type discovery inTypeFixupSignatureto enqueue the node.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj | Adds the new VirtualMethodUseNode source file to the project. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/VirtualMethodUseNode.cs | New dependency-analysis marker node representing a used non-GVM virtual slot. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs | Adds a node cache + factory method for VirtualMethodUseNode. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs | Expands type discovery to add InheritedVirtualMethodsNode when virtual slots/interfaces are present. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs | Marks non-GVM virtual slots as used for VirtualEntry fixups to enable conditional compilation. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/InheritedVirtualMethodsNode.cs | Adds conditional static dependencies to compile implementations based on VirtualMethodUseNode conditions (class + interface paths). |
68f23ac to
f924912
Compare
We determine the full set of used generic methods by tracking all types from the app via InheritedVirtualMethodsNode and GVM callsites via GVMDependencies node. The GVMDependencies node then tries to resolve the method on all types from the app, which are a dynamic dependency for this node. This only handles generic methods so, non-generic methods on generic types are not resolved with this mechansim. This PR completes the support. This is done by adding conditional dependencies to InheritedVirtualMethodsNode. For a generic type, we determine all virtual/interface method slots and the resolved target method. Then we add a conditional dependency that says that the resolved method is included if the slot defining method is used in the app. We detect if the slot defining method is used by creating a VirtualMethodUseNode at callsites.
It serves the same purpose both for R2R and NativeAOT
f924912 to
ff3d0a6
Compare
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/coreclr/tools/Common/Compiler/DependencyAnalysis/VirtualMethodUseNode.cs:22
- Changing
VirtualMethodUseNodetopublicintroduces new public surface area in compiler infrastructure. Unless this is intentionally a supported API, it should stayinternal(and ideally be hidden behind a base return type) to avoid committing to it long-term and to avoid requiring API-approval tracking.
|
|
||
| private NodeCache<MethodDesc, VirtualMethodUseNode> _virtualMethodUseNodes; | ||
|
|
||
| public VirtualMethodUseNode VirtualMethodUse(MethodDesc method) |
| if (!_typeDesc.IsGenericDefinition && | ||
| !_typeDesc.IsInterface && | ||
| _typeDesc.IsDefType && | ||
| (factory.CompilationCurrentPhase == 0) && | ||
| factory.CompilationModuleGroup.VersionsWithType(_typeDesc) && | ||
| TypeHasGVMSlots(_typeDesc)) | ||
| factory.CompilationModuleGroup.VersionsWithType(_typeDesc)) |
| // For non-GVM virtual method calls, mark the virtual slot as used so that | ||
| // conditional dependencies on InheritedVirtualMethodsNode can trigger compilation | ||
| // of concrete implementations. | ||
| if (_fixupKind == ReadyToRunFixupKind.VirtualEntry && | ||
| Method.IsVirtual && |
| public override IEnumerable<CombinedDependencyListEntry> SearchDynamicDependencies(List<DependencyNodeCore<NodeFactory>> markedNodes, int firstNode, NodeFactory context) => null; | ||
|
|
||
| public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDependencies(NodeFactory factory) | ||
| { |
There was a problem hiding this comment.
@MichalStrehovsky this would be the equivalent of EETypeNode.GetConditionalStaticDependencies. The code here share some logic with it as well as with the resolving of target method from GVMDependenciesNode. Unfortunately, I couldn't see an easy way to reuse the pieces from there, also without degrading clarity.
We determine the full set of used generic methods by tracking all types from the app via InheritedVirtualMethodsNode and GVM callsites via GVMDependencies node. The GVMDependencies node then tries to resolve the method on all types from the app, which are a dynamic dependency for this node. This only handles generic methods so, non-generic methods on generic types are not resolved with this mechansim.
This PR completes the support. This is done by adding conditional dependencies to InheritedVirtualMethodsNode. For a generic type, we determine all virtual/interface method slots and the resolved target method. Then we add a conditional dependency that says that the resolved method is included if the slot defining method is used in the app. We detect if the slot defining method is used by creating a VirtualMethodUseNode at callsites.
Methods failing to compile before, from this set of tests: https://gist.github.com/BrzVlad/7ea5987ec494705ac7b4b6aaf1325fe7