-
Notifications
You must be signed in to change notification settings - Fork 5.3k
ILC: track concrete dependencies of open generic methods #122012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
This reverts commit dc8058b.
1b40155 to
74c0072
Compare
d40b283 to
5d9b2bc
Compare
To fix case without method generic parameters
No longer needed since the conversion of generic unboxing thunks to the underlying target method no longer happens here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the ILC (IL Compiler) to track concrete dependencies of open generic methods to prevent compilation failures when RyuJit inlines generic callees into callsites with concrete generic arguments. The key change is expanding the scope of dependency tracking from only concrete generic methods to include partially instantiated (open) generic methods that may still have concrete dependencies.
- Renamed
ShadowConcreteMethodNodetoShadowGenericMethodNodeto reflect its expanded purpose - Added tracking for open generic methods in three critical entry points: reflection access, generic virtual method (GVM) calls, and direct method calls
- Enhanced
ReadyToRunGenericHelperNode.InstantiateDependenciesto handle non-concrete instantiations by attempting substitution and tracking shadow generic methods when results remain canonical
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ShadowGenericMethodNode.cs |
Renamed from ShadowConcreteMethodNode.cs; removed assertion that restricted usage to concrete methods only, allowing tracking of open generic methods |
ReadyToRunGenericHelperNode.cs |
Added logic to handle non-concrete instantiations by checking substitution results and tracking shadow generic methods when instantiations remain canonical |
ReflectionInvokeMapNode.cs |
Added tracking of shadow generic methods for shared generic instantiations accessed via reflection |
GenericVirtualMethodImplNode.cs |
Added shadow generic method dependency tracking for generic virtual method implementations |
GenericDictionaryNode.cs |
Updated references from ShadowConcreteMethod to ShadowGenericMethod |
NodeFactory.cs |
Renamed method and hashtable from ShadowConcreteMethod to ShadowGenericMethod; updated CanonicalEntrypoint method |
ILImporter.Scanner.cs |
Changed direct call tracking to use ShadowGenericMethod instead of CanonicalEntrypoint |
MetadataManager.cs |
Updated type cast from ShadowConcreteMethodNode to ShadowGenericMethodNode |
ILScanner.cs |
Updated type cast from ShadowConcreteMethodNode to ShadowGenericMethodNode |
RyuJitCompilation.cs |
Updated type cast from ShadowConcreteMethodNode to ShadowGenericMethodNode |
ILCompiler.Compiler.csproj |
Updated file reference from ShadowConcreteMethodNode.cs to ShadowGenericMethodNode.cs |
ILCompiler.ReadyToRun.csproj |
Updated file reference from ShadowConcreteMethodNode.cs to ShadowGenericMethodNode.cs |
Generics.cs |
Added five comprehensive test cases covering GVM inlining with different inheritance patterns and reflection scenarios |
...lr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs
Outdated
Show resolved
Hide resolved
…nalysis/GenericVirtualMethodImplNode.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/coreclr/tools/Common/Compiler/DependencyAnalysis/ShadowGenericMethodNode.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/Common/Compiler/DependencyAnalysis/ShadowConcreteMethodNode.cs
Show resolved
Hide resolved
...clr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs
Outdated
Show resolved
Hide resolved
...clr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs
Outdated
Show resolved
Hide resolved
- Split ShadowGenericMethodNode into: - common base class ShadowMethodNode - existing ShadowConcreteMethodNode - new ShadowNonConcreteMethodNode with specific asserts in the derived classes - fix comment - pass isConcreteInstantiation through InstantiateDependencies and GetTarget - limit creation of ShadowNonConcreteMethodNode to where AcquiresInstMethodTableFromThis is true - move instantiation logic for non-concrete instantiations into GetTarget implementations
InstantiateDependencies here could be called for non-concrete instantiations even before these changes.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs
Outdated
Show resolved
Hide resolved
…ndleCallAction.cs
And limit direct call shadow nodes We already create concrete nodes inside CanonicalEntrypoint
MichalStrehovsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good otherwise! I'll merge main into this so I can run size measurements over in rt-sz, just in case something unexpected pops up.
src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs
Outdated
Show resolved
Hide resolved
...clr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs
Outdated
Show resolved
Hide resolved
...clr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/Common/Compiler/DependencyAnalysis/ShadowNonConcreteMethodNode.cs
Show resolved
Hide resolved
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Ah, I misinterpreted what rt-sz was complaining about. You already merged main before, so we had #122227 in this brach, but the SDK produced from dotnet/dotnet still doesn't have this, so rt-sz is failing with |
- Remove dead code - Remove redundant check - Improve comment
When RyuJit inlines a generic callee into a callsite where some of the generic arguments are concrete, the code generation may require concrete dependencies of the inlinee. We need to ensure that the scanner tracks such dependencies statically so that they are available for codegen.
For example:
Here the concrete instantiation
Caller<string>is not discovered statically - but whenCallee<object, T>()is inlined into the canonical code forCaller<__Canon>, this results in a concrete reference toContainer<object>fromCaller<__Canon>, so we need to keep any generic dictionary dependencies ofCallee<object, T>.This change adds additional tracking of open generic methods to ensure that any concrete dependencies are preserved during scanning. It adds tracking similar to what's done by
ShadowConcreteMethodNode- now there's a newShadowNonConcreteMethodNodethat tracks non-concrete (not fully instantiated) generics. There are three entry points where we needed additional tracking for open generics:Now when
ShadowNonConcreteMethodNodeis used to instantiate generic dependencies of runtime-determined nodes, the resulting instantiation may not be concrete. We handle this by attempting the instantiation, and if the result is concrete we track the dependency as normal. If the result is not concrete, then we either track it as anotherShadowNonConcreteMethodNode(if the result is a generic method) or ignore it.Fixes #120847