Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates the CoreCLR R2R compilation toolchain to allow devirtualization of interface calls on arrays by recognizing implicitly-implemented array interfaces and resolving the actual target methods on System.SZArrayHelper.
Changes:
- Extend the well-known type system to include
SZArrayHelperand key generic collection interfaces used by arrays. - Teach
MetadataTypeSystemContextto resolve well-known types across multiple namespaces (e.g.,System.Collections.Generic). - Enable R2R devirtualization for array interface calls by removing the previous R2R block and adding a
SZArrayHelper-based resolution path inDevirtualizationManager.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/Common/TypeSystem/Common/WellKnownType.cs | Adds well-known type enum entries for SZArrayHelper and array-relevant generic interfaces. |
| src/coreclr/tools/Common/TypeSystem/Common/TypeDesc.cs | Classifies the new well-known types as Class/Interface for cached type flag computation. |
| src/coreclr/tools/Common/TypeSystem/Common/MetadataTypeSystemContext.cs | Switches well-known type table to (Namespace, TypeName) to support non-System namespaces during system module initialization. |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Removes the R2R-time “array interface devirt not supported” early-fail, allowing devirtualization to proceed. |
| src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs | Adds array-interface recognition and maps eligible interface methods on System.Array to instantiated SZArrayHelper methods. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs:93
- GetActualImplementationForArrayGenericIListOrIReadOnlyListMethod assumes SZArrayHelper and the target method are always present, and will throw (TypeLoadException / NullReferenceException) if they aren't. Since well-known types are optionally present on some profiles, this should fail devirtualization gracefully instead of failing the compilation. This also fixes the typo "theT's" -> "T's" in the comment.
MethodDesc genericImplementor = declMethod.Context.GetWellKnownType(WellKnownType.SZArrayHelper).GetMethod(declMethod.Name, null);
Debug.Assert(genericImplementor != null);
// OPTIMIZATION: For any method other than GetEnumerator(), we can safely substitute
// "Object" for reference-type theT's. This causes fewer methods to be instantiated.
if (genericImplementor.Name != "GetEnumerator"u8 && !typeParam.IsValueType)
src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs:126
- If GetActualImplementationForArrayGenericIListOrIReadOnlyListMethod can't locate SZArrayHelper (or the target method), the array-interface fast-path should set an explicit failure detail and return null, rather than returning a null MethodDesc and failing later.
return GetActualImplementationForArrayGenericIListOrIReadOnlyListMethod(declMethod, resultElemType);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/tools/Common/TypeSystem/Common/MetadataTypeSystemContext.cs:55
MetadataTypeSystemContext.WellKnownTypeNamesis a public API, and this PR changes its type fromIEnumerable<string>to a tuple enumerable. That’s an API-breaking change for any out-of-repo consumers of the type system libraries.
To avoid breaking existing callers, keep WellKnownTypeNames with the original IEnumerable<string> shape and introduce a new property for (Namespace, TypeName) pairs (and update in-repo callers to use the new property).
public static IEnumerable<(string Namespace, string TypeName)> WellKnownTypeNames => s_wellKnownTypeNames;
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core R2R devirtualization behavior and JIT-interface context handling in a way that can affect codegen broadly, so it warrants final human review despite looking consistent with existing CoreCLR mechanisms.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
PTAL |
davidwrighton
left a comment
There was a problem hiding this comment.
Looks almost good to me. I'm curious if the new R2RTest infrastructure would let you write a test that validated that devirtualization happened for this case.
There was a problem hiding this comment.
🟡 Changes recommended
The new logic can classify multidimensional or pointer arrays as SZ arrays after type normalization and emit invalid direct helper calls.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Variant array casts may resolve to an SZArrayHelper instantiation without a compiled body or dependency.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Resolved since last review (1)
Noticed while working on #131265. We were not inlining `GetEnumerator` here under NativeAOT, leading to foreach over array interfaces not able to be inlined even after devirtualization.
I didn't know about the new R2RTest infrastructure, could you elaborate a bit? (searching |
|
We have some tests under https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests that let us programmatically validate ReadyToRun outputs. We've started adding tests for some cases. |
|
I added the tests for it. PTAL. |

Support array interface devirtualization in R2R.
When we see an interface method on
System.Array, we check if it's an implicitly implemented array interface. For implicitly implemented array interfaces we get the actual implementation fromSZArrayHelper.Example:
Before:
After:
cc: @MichalStrehovsky @davidwrighton