[NO REVIEW] Move ArgIterator/TransitionBlock to tools/Common for cDAC reuse#128460
[NO REVIEW] Move ArgIterator/TransitionBlock to tools/Common for cDAC reuse#128460max-charlamb wants to merge 7 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR relocates the ReadyToRun calling-convention helpers (ArgIterator/TransitionBlock) into src/coreclr/tools/Common/CallingConvention/ and updates ILCompiler.ReadyToRun.csproj to consume them via Link includes, enabling shared reuse across tooling.
Changes:
- Added
ArgIterator.csandTransitionBlock.csundertools/Common/CallingConvention/. - Updated
ILCompiler.ReadyToRun.csprojto link these files from the new Common location and removed the old per-project compile includes. - (Review note) Found a likely copy/paste bug in
TransitionBlock.ComputeReturnValueTreatmentfor SystemV AMD64 16-byte struct return encoding (second eightbyte classification check).
Reviewed changes
Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/tools/Common/CallingConvention/ArgIterator.cs | Relocated ArgIterator implementation into tools/Common for reuse. |
| src/coreclr/tools/Common/CallingConvention/TransitionBlock.cs | Relocated TransitionBlock implementation into tools/Common for reuse (also contains a likely SystemV return-encoding bug). |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj | Switched to Link-based compilation from the new Common location and removed old compile entries. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/coreclr/tools/Common/CallingConvention/ArgIterator.cs:187
ArgIteratorDatanow stores arguments/return types asITypeHandle. Since the concrete crossgen2 implementation (TypeHandle) is a struct, assigning it into anITypeHandle[]boxes each element, adding per-parameter allocations and virtual dispatch in what is typically a hot path during compilation.
If the intent is to keep this code reusable without regressing crossgen2 perf, consider making the calling-convention pipeline generic over TTypeHandle (e.g., where TTypeHandle : struct, ITypeHandle) so the ReadyToRun build stays allocation-free, while still allowing a separate cDAC-backed struct implementation later.
| private static readonly int[] s_elemSizes = new int[] | ||
| { | ||
| 0, //ELEMENT_TYPE_END 0x0 | ||
| 0, //ELEMENT_TYPE_VOID 0x1 | ||
| 1, //ELEMENT_TYPE_BOOLEAN 0x2 |
| public bool Equals(TypeHandle other) | ||
| { | ||
| return _isByRef == other._isByRef && _type == other._type; | ||
| } | ||
|
|
||
| public override int GetHashCode() { return (int)_type.GetHashCode(); } | ||
|
|
| // Check to see if this argument lowers to a byref on the wasm side | ||
| TypeHandle typeHandle; | ||
| ITypeHandle typeHandle; | ||
| argit.GetArgType(out typeHandle); | ||
| if (WasmLowering.LowerToAbiType(typeHandle.GetRuntimeTypeHandle()) == null) | ||
| if (WasmLowering.LowerToAbiType(((TypeHandle)typeHandle).GetRuntimeTypeHandle()) == null) | ||
| { |
|
/run |
|
/azp run runtime-coreclr r2r |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
|
/azp run runtime-coreclr r2r |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GCRefMapBuilder.cs:95
parameterTypeswas changed toITypeHandle[], but the stored values arenew TypeHandle(...)whereTypeHandleis a struct. Storing structs into an interface-typed array causes boxing/allocation per element, which could be a non-trivial regression in crossgen2 sinceBuildArgIteratorruns per method signature.
bool isVarArg = false;
TypeHandle returnType = new TypeHandle(signature.ReturnType);
ITypeHandle[] parameterTypes = new ITypeHandle[signature.Length];
for (int parameterIndex = 0; parameterIndex < parameterTypes.Length; parameterIndex++)
{
parameterTypes[parameterIndex] = new TypeHandle(signature[parameterIndex]);
}
CallingConventions callingConventions = (hasThis ? CallingConventions.ManagedInstance : CallingConventions.ManagedStatic);
|
/azp run runtime-coreclr r2r |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
Move ArgIterator.cs and TransitionBlock.cs from the ILCompiler.ReadyToRun project directory into src/coreclr/tools/Common/CallingConvention/ to enable future reuse by the cDAC. This is a pure file relocation with no code changes. The namespace remains ILCompiler.DependencyAnalysis.ReadyToRun. The csproj is updated to use file-link includes from the new Common location. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Create ITypeHandle interface in CallingConvention/ with all type queries needed by ArgIterator and TransitionBlock - Extract TypeHandle struct from ArgIterator.cs into TypeHandle.cs - Move GetElemSize helper to static method on ITypeHandle - Replace all GetRuntimeTypeHandle() escape hatches in ArgIterator (3) and TransitionBlock (3) with ITypeHandle method calls - Port all TypeHandle references in ArgIterator/TransitionBlock to ITypeHandle (fields, parameters, locals, return types) - TypeHandle retains GetRuntimeTypeHandle() for crossgen2-only code (WasmLowering) but it is not part of the interface Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TypeHandle wraps TypeDesc which is crossgen2-specific, so it belongs in the ReadyToRun project rather than in the shared Common directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…amespace and remove TypeSystemContext dependency - Change namespace from ILCompiler.DependencyAnalysis.ReadyToRun to Internal.Runtime.CallingConvention for the shared files (ArgIterator, TransitionBlock, ITypeHandle) - Remove TypeSystemContext from ArgIterator constructor; pass TransitionBlock directly along with isWindows, objectTypeHandle, and intPtrTypeHandle parameters - Update all consumer files with new using and ArgIterator alias to resolve ambiguity with System.ArgIterator Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…peHandle adapter - Change namespace to Internal.CallingConvention - Replace TargetDetails with individual params in TransitionBlock.FromTarget - Extract standalone SystemV/FpStruct types for cDAC use - Refactor ReportPointersFromStructInRegisters to use ITypeHandle - Add pragma suppressions for crossgen2 analyzer warnings - Add CdacTypeHandle adapter wrapping IRuntimeTypeSystem + TypeHandle - Add ICallingConvention contract with EnumerateCallerStackRefs - Add CallingConvention_1 implementation using shared ArgIterator - Wire shared CallingConvention files into cDAC project TODO: VarArgs support in CallingConvention contract Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…umeration - Implement EnumerateCallerStackRefs in CallingConvention_1 to walk caller stack frames and report GC references for object refs, byrefs, and structs - Add EnumerateValueTypeGCRefs using GCDesc series to enumerate GC pointers within unboxed value types on the stack - Scope to AMD64 Windows; throw NotImplementedException for SystemV struct- in-registers, HFA, FP struct, x86 trivial structs, and WASM field alignment - Update CdacTypeHandle stubs with NotImplementedException for unimplemented platform-specific callbacks - Add 10 unit tests for GCDesc-based value type GC reference enumeration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace GC-specific CallerStackGCRef/EnumerateCallerStackRefs with general-purpose ArgumentLocation/EnumerateArguments on ICallingConvention - Wire GcScanner.PromoteCallerStack to use ICallingConvention.EnumerateArguments instead of duplicate signature-decoding logic - Add ReportPointersFromValueType for GCDesc-based struct GC walking - Add IsUnboxingStub to IRuntimeTypeSystem for correct value-type this interior pointer detection (matches native: IsValueType && !IsUnboxingStub) - Move CdacTypeHandle.cs to CallingConvention folder - Remove CallingConventionTests.cs (was GCDesc-specific, needs rewrite) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
max-charlamb
force-pushed
the
cdac-shared-argiterator
branch
from
5c05657 to
d4256bf
Compare
|
Note This review was generated by Copilot. 🤖 Copilot Code Review — PR #128460Holistic AssessmentMotivation: Sharing crossgen2's Approach: The Summary: Detailed Findings
|
|
/azp run crossgen outerloop |
|
No pipelines are associated with this pull request. |
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // LoongArch64/Wasm alignment | ||
| int GetFieldAlignment(); | ||
|
|
||
| private static readonly int[] s_elemSizes = new int[] |
| private readonly RuntimeInfoArchitecture _arch; | ||
| private readonly RuntimeInfoOperatingSystem _os; | ||
|
|
||
| public CdacTypeHandle(TypeHandle typeHandle, Target target) | ||
| { | ||
| _typeHandle = typeHandle; | ||
| _target = target; | ||
| _arch = _target.Contracts.RuntimeInfo.GetTargetArchitecture(); | ||
| _os = _target.Contracts.RuntimeInfo.GetTargetOperatingSystem(); | ||
| } |
| IRuntimeTypeSystem rts = _target.Contracts.RuntimeTypeSystem; | ||
| ICallingConvention cc = _target.Contracts.CallingConvention; | ||
| MethodDescHandle mdh = rts.GetMethodDescHandle(methodDescPtr); | ||
|
|
||
| MethodSignature<GcTypeKind> methodSig; | ||
| try | ||
| foreach (ArgumentLocation arg in cc.EnumerateArguments(mdh)) |
| /// <summary> | ||
| /// Describes the location of an argument on a caller's transition frame, | ||
| /// produced by walking the callee's method signature with ArgIterator. | ||
| /// </summary> | ||
| public readonly struct ArgumentLocation | ||
| { | ||
| /// <summary>Byte offset from the start of the transition block.</summary> | ||
| public int Offset { get; init; } | ||
|
|
||
| /// <summary>The CorElementType of this argument (Class, ValueType, Byref, I4, etc.).</summary> | ||
| public CorElementType ElementType { get; init; } | ||
|
|
||
| /// <summary>The TypeHandle for this argument's type (needed for struct GC walking).</summary> | ||
| public TypeHandle TypeHandle { get; init; } | ||
|
|
||
| /// <summary>True if this is the "this" pointer slot.</summary> | ||
| public bool IsThis { get; init; } | ||
|
|
||
| /// <summary>True if this is a value type "this" (passed as interior pointer).</summary> | ||
| public bool IsValueTypeThis { get; init; } | ||
|
|
||
| /// <summary>True if this slot holds a generic instantiation parameter (MethodTable* or MethodDesc*).</summary> | ||
| public bool IsParamType { get; init; } | ||
|
|
||
| /// <summary>True if this argument is a struct passed by reference (e.g., large struct on AMD64).</summary> | ||
| public bool IsPassedByRef { get; init; } | ||
| } | ||
|
|
||
| public interface ICallingConvention : IContract | ||
| { | ||
| static string IContract.Name => nameof(CallingConvention); | ||
|
|
||
| /// <summary> | ||
| /// Enumerate argument locations on the caller's transition frame for the given method. | ||
| /// This uses the shared ArgIterator to walk the method signature and determine | ||
| /// where each argument resides (stack offset, element type, type handle). | ||
| /// The caller is responsible for interpreting these locations for GC or other purposes. | ||
| /// </summary> | ||
| IEnumerable<ArgumentLocation> EnumerateArguments(MethodDescHandle methodDesc) => throw new System.NotImplementedException(); | ||
| } |
|
Closing -- outdated. Will revisit if the ArgIterator/TransitionBlock move becomes necessary for a concrete cDAC reuse case. |
Note
This PR was created with Copilot assistance.
Summary
Share crossgen2's
ArgIteratorandTransitionBlockwith the cDAC by moving them totools/Common/CallingConvention/, introducing anITypeHandleinterface abstraction, and building a newCallingConventioncontract that enumerates GC references on caller stack frames.Changes
Shared calling convention infrastructure (commits 1-4)
ArgIterator.csandTransitionBlock.csfromILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/totools/Common/CallingConvention/ITypeHandleinterface -- abstraction over type info needed byArgIterator(size, CorElementType, SystemV classification, HFA, etc.)TypeHandlestruct to its own file and implementITypeHandleTypeSystemContextdependency fromTransitionBlock.FromTarget()-- decompose into(TargetArchitecture, isWindows, isApplePlatform, isArmel)parameters to avoid pulling in the full TypeSystem dependency chainInternal.CallingConventionfor shared consumptioncDAC CallingConvention contract (commits 5-6)
ICallingConventioncontract withEnumerateCallerStackRefsAPICallingConvention_1implementation -- uses the sharedArgIteratorto walk arguments and report GC references (object refs, byrefs, and struct fields via GCDesc)CdacTypeHandleadapter -- implementsITypeHandlebacked by the cDAC'sIRuntimeTypeSystem; throwsNotImplementedExceptionfor unimplemented platform-specific callbacks (SystemV struct-in-registers, HFA, FP structs, x86 trivial structs)EnumerateValueTypeGCRefs-- walks GCDesc series to enumerate GC pointers within unboxed value types on the stackNotImplementedExceptionFile-link structure