Rename AsyncMethodDesc to AsyncMethodVariant, add "AsyncVariant" flag to MethodWithToken and MethodWithGCInfo #121218

jtschuster · 2025-10-30T21:54:10Z

AsyncMethodVariant is isolated to CorInfoImpl.cs and only used in jit compilation.

MethodWithGCInfo and MethodWithToken (Code and R2R method signature providing nodes, respectively) have an additional AsyncVariant flag. When constructed in CorInfoImpl, an AsyncMethodVariant instance should unwrap the underlying method and pass AsyncVariant=true.

AsyncMethodKind is also added to MethodDesc to determine whether the method is AsyncCallConv or not, TaskReturning or void-returning, and thunk or impl.

…sc classes - EcmaMethod represents the variant encoded in metadata, either async or Task-returning. The Signature is adjusted accordingly to align with the calling convention. - AsyncMethodDesc is replaced with AsyncMethodThunk, which is the Async variant thunk of a Task-returning implementation method. - TaskReturningAsyncThunk is the variant of an async implementation method - AsyncMethodData was borrowed from the runtime implementation and added to the managedTypeSystem. - IsTaskReturning is now on MethodDesc and uses the AsyncMethodData to determine whether the type returns Task/ValueTask. It has different semantics than the runtime method of the same name, as this returns true for Async methods too. - GetAsyncOtherVariant() is also borrowed from the runtime implementation. This makes the factory unnecessary, as the unique "other variant" can be retrieved from the definition without creating a new instance each time.

Copilot

Pull Request Overview

This PR refactors the async method handling infrastructure in the CoreCLR AOT compiler's type system. It moves async method variant management from the JIT interface layer into the core type system, making async method thunks first-class citizens with proper lifetimes.

Key changes:

Moved async method thunk creation from JitInterface to the core type system with new AsyncMethodThunk and TaskReturningAsyncThunk classes
Introduced AsyncMethodData and AsyncMethodKind to track method async characteristics and signatures
Updated EcmaMethod, MethodForInstantiatedType, and InstantiatedMethod to support async variant tracking

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
ILCompiler.TypeSystem.csproj	Adds new async thunk source files to the project
ILCompiler.ReadyToRun.csproj	Removes old JitInterface async method files
EcmaMethod.cs	Adds async method data tracking and variant generation, reformats constants
TaskReturningAsyncThunk.cs	New thunk class for async-callable variants of Task-returning methods
MethodForInstantiatedType.cs	Adds async variant support for instantiated type methods
MethodDesc.cs	Adds core async infrastructure: `ReturnsTaskOrValueTask()`, `CreateAsyncSignature()`, `AsyncMethodData`, `AsyncMethodKind`, `GetAsyncOtherVariant()`
MethodDelegator.cs	Makes `AsyncMethodData` abstract for delegator implementations
InstantiatedMethod.cs	Adds async variant support for generic method instantiations
AsyncMethodThunk.cs	New thunk class for async variants of Task-returning methods
AsyncMethodDescFactory.cs	Deleted - factory no longer needed with new approach
AsyncMethodDesc.cs	Deleted - replaced by new thunk classes in type system

src/coreclr/tools/Common/TypeSystem/Common/TaskReturningAsyncThunk.cs

src/coreclr/tools/Common/TypeSystem/Common/AsyncMethodThunk.cs

src/coreclr/tools/Common/TypeSystem/Common/TaskReturningAsyncThunk.cs

src/coreclr/tools/Common/TypeSystem/Common/AsyncMethodThunk.cs

src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.cs

src/coreclr/tools/Common/TypeSystem/Common/InstantiatedMethod.cs

Copilot

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

src/coreclr/tools/Common/TypeSystem/Ecma/EcmaMethod.cs

src/coreclr/tools/Common/TypeSystem/Common/TaskReturningAsyncThunk.cs

Copilot

Pull Request Overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

src/coreclr/tools/Common/TypeSystem/Common/InstantiatedMethod.Async.cs

MichalStrehovsky · 2025-10-31T09:47:17Z

What considerations lead to turning EcmaMethod into the "impl" variant of async? I understand there are going to be warts no matter how we implement this, but on the surface EcmaMethod not having the signature that it has in the metadata will lead to a lot of warts. My instinct would be to keep the signature as-is.

This is not the first time we're in a situation where a single method can have different entrypoints with different signature. We have similar situation with unboxing thunks (R2R + NAOT), instantiating thunks (R2R), special default interface method instantiating thunks (NAOT), or even p/invokes (NAOT, distinguishing the marshalled and "naked" entrypoints),

In the CoreCLR type system the distinction between "entry point" and "method" is kind of fuzzy (even before async) - it's all kinds of MethodDescs.

In our managed compilers, we represent entry points using ISymbolNode - often actual code bytes that one can refer to with a reloc. Methods are represented with MethodDescs. Typically there's just one MethodDesc for a single method (modulo generics). Sometimes we make more than one MethodDesc for a single method, but those are always short lived and don't leak out from the component that knows how to handle it (like the MethodDesc that is getting deleted in this PR). We don't want to end up with these "weird" MethodDescs in parts of the system that shouldn't have to deal with the weirdness.

For example, with unboxing thunks, most of the system doesn't care about their existence. Some parts of the system deal with them by carrying a pair of MethodDesc + bool. And we have a MethodDesc for them that is local to the JitInterface but doesn't enter the rest of the compiler.

In native AOT, there's also an instantiating unboxing MethodDesc that is similar to how I think of async so far:

runtime/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/DependencyAnalysis/RyuJitNodeFactory.cs

Lines 23 to 30 in 9ebda3d

    
           if (TypeSystemContext.IsSpecialUnboxingThunkTargetMethod(method)) 
        
           { 
        
               return MethodEntrypoint(TypeSystemContext.GetRealSpecialUnboxingThunkTargetMethod(method)); 
        
           } 
        
           else if (TypeSystemContext.IsDefaultInterfaceMethodImplementationThunkTargetMethod(method)) 
        
           { 
        
               return MethodEntrypoint(TypeSystemContext.GetRealDefaultInterfaceMethodImplementationThunkTargetMethod(method)); 
        
           }

This one also exists, but its existence is pretty limited because very few components will get to the MethodDesc itself (we basically only hand it out to RyuJIT).

It was added in dotnet/corert#2566 and the implementation is still pretty much the same.

The runtime async methods look like normal method when looking at them in IL. The callsites to them look regular. They are weird when one looks at the method body. So my instinct would be not to make EcmaMethod for these methods weird right off the bat. Most components will probably not care about handling these specially. If we make the signature on EcmaMethod weird, suddenly a lot of components that don't really need to deal with async will have to deal with async. For example:

Resolving a MemberRef that was created before runtime async into a definition that is runtime async will need special casing in signature comparisons:

runtime/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaType.cs

Lines 321 to 336 in 9ebda3d

    
           public override EcmaMethod GetMethod(ReadOnlySpan<byte> name, MethodSignature signature, Instantiation substitution) 
        
           { 
        
               var metadataReader = this.MetadataReader; 
        
               foreach (var handle in _typeDefinition.GetMethods()) 
        
               { 
        
                   if (metadataReader.StringEquals(metadataReader.GetMethodDefinition(handle).Name, name)) 
        
                   { 
        
                       var method = _module.GetMethod(handle, this); 
        
                       if (signature == null || signature.Equals(method.Signature.ApplySubstitution(substitution))) 
        
                           return method; 
        
                   } 
        
               } 
        
               return null; 
        
           }

Reflection metadata generation will need to undo the async transform:

runtime/src/coreclr/tools/aot/ILCompiler.MetadataTransform/ILCompiler/Metadata/Transform.Method.cs

Lines 159 to 178 in 9ebda3d

    
           public override MethodSignature HandleMethodSignature(Cts.MethodSignature signature) 
        
           { 
        
               // TODO: if Cts.MethodSignature implements Equals/GetHashCode, we could enable pooling here. 
        
               var result = new MethodSignature 
        
               { 
        
                   CallingConvention = GetSignatureCallingConvention(signature), 
        
                   GenericParameterCount = signature.GenericParameterCount, 
        
                   ReturnType = HandleType(signature.ReturnType), 
        
                   // TODO-NICE: VarArgParameters 
        
               }; 
        
               result.Parameters.Capacity = signature.Length; 
        
               for (int i = 0; i < signature.Length; i++) 
        
               { 
        
                   result.Parameters.Add(HandleType(signature[i])); 
        
               } 
        
               return result; 
        
           }

We'll need to special case async in virtual method resolution. Etc.

My initial instinct would be to leave the metadata as it is in the IL file and make very local MethodDescs as needed. And extra flags where needed:

runtime/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs

Line 964 in 9ebda3d

    
           public DynamicHelperCellKey(MethodWithToken method, bool isUnboxingStub, bool isInstantiatingStub)

jkotas · 2025-10-31T12:58:40Z

src/coreclr/tools/Common/TypeSystem/Ecma/EcmaMethod.cs

+            _metadataSignature = signature;
        }

-        public override MethodSignature Signature


I agree with @MichalStrehovsky . This should stay as is.

The managed type system "features" have been designed as incremental. E.g. adding *CodeGen* or *Sorting* does not change how any of the core methods work. This change would be violating this design.

… and flags elsewhere.

jkotas · 2025-11-04T02:07:13Z

src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs

        READYTORUN_METHOD_SIG_Constrained = 0x20,
        READYTORUN_METHOD_SIG_OwnerType = 0x40,
        READYTORUN_METHOD_SIG_UpdateContext = 0x80,
+        READYTORUN_METHOD_SIG_AsyncThunkVariant = 0x100,


Should this be just AsyncVariant (ie cover both Thunk or impl)?

This enum is part of R2R file format. It is used to encode references to methods in other assemblies. This encoding should be resilient to compatible changes in the implementation of other assemblies. If we encode AsyncThunkVariant here, it does not sound like we will be resilient to the implementations in other assemblies changing between async v1 and async v2.

Is READYTORUN_METHOD_SIG_SlotInsteadOfToken used? Could we reuse it (bumping the R2R version of course)?

jkotas · 2025-11-04T02:11:09Z

src/coreclr/tools/Common/TypeSystem/Common/InstantiatedMethod.cs


 using System;
 using System.Diagnostics;
+using System.Threading;


Are the changes in this file still needed?

MichalStrehovsky · 2025-11-04T06:47:01Z

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

            get
            {
+#if READYTORUN
+                return _methodCodeNode.GetJitMethod(_asyncMethodFactory);


Would it be better to inline the contents of GetJitMethod to here? Presumably MethodWithGCInfo doesn't really need to know about AsyncMethodVariantFactory.

MichalStrehovsky · 2025-11-04T06:47:38Z

src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs

        READYTORUN_METHOD_SIG_Constrained = 0x20,
        READYTORUN_METHOD_SIG_OwnerType = 0x40,
        READYTORUN_METHOD_SIG_UpdateContext = 0x80,
+        READYTORUN_METHOD_SIG_AsyncThunkVariant = 0x100,


Is READYTORUN_METHOD_SIG_SlotInsteadOfToken used? Could we reuse it (bumping the R2R version of course)?

MichalStrehovsky · 2025-11-04T06:49:53Z

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

            if (info->pResolvedTokenVirtualMethod != null)
            {
-                methodWithTokenDecl = ComputeMethodWithToken(decl, ref *info->pResolvedTokenVirtualMethod, null, false);
+                methodWithTokenDecl = ComputeMethodWithToken(decl, ref *info->pResolvedTokenVirtualMethod, null, false, asyncVariant: false);


I think the asyncVariant: false here are not quite correct (applies to all uses in this method)

I would make this throw RequiresRuntimeJitException if decl.IsAsync or impl.IsAsync. for now.

Ah good idea, I was putting false for places I wasn't sure (since we don't even try to compile async methods yet), but throwing would make more sense.

MichalStrehovsky · 2025-11-04T06:52:04Z

src/coreclr/tools/Common/TypeSystem/Common/AsyncMethodVariant.cs

+        {
+            get
+            {
+                return _asyncSignature ??= _wrappedMethod.Signature.CreateAsyncSignature();


Inline CreateAsyncSignature to here? It doesn't feel like a MethodSignature concern.

MichalStrehovsky · 2025-11-04T06:55:10Z

src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.Async.cs

+        private static class MethodSignatureFlagsExtensions
+        {
+            public const MethodSignatureFlags ReturnsTaskOrValueTask = (MethodSignatureFlags)(1 << 30); // Not a real flag, just a cached value
+            public const MethodSignatureFlags ReturnsTaskOrValueTaskMask = (MethodSignatureFlags)(1 << 31); // Not a real flag, just a cached value
+        }


This cache might be premature, I wouldn't do it yet.

I think we already have a bug that CreateAsyncSignature will reuse the cache portion of flags despite updating ReturnType. Stashing bits in the flags is problematic.

MichalStrehovsky · 2025-11-04T07:57:26Z

src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.Async.cs

+        public bool IsAsyncVariant
+        {
+            get
+            {
+                return AsyncMethodKind is
+                AsyncMethodKind.AsyncVariantImpl or
+                AsyncMethodKind.AsyncVariantThunk;
+            }
+        }
+


Is this used? Looks like we also have an extension method with the same name. Maybe the extension method is not needed?

MichalStrehovsky · 2025-11-04T07:58:26Z

src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.Async.cs

+        /// <summary>
+        /// Is this method callable as an async method? (i.e. uses Async calling convention)
+        /// </summary>
+        public bool IsAsyncCallConv
+        {
+            get
+            {
+                return AsyncMethodKind is
+                AsyncMethodKind.AsyncVariantImpl or
+                AsyncMethodKind.AsyncVariantThunk or
+                AsyncMethodKind.AsyncExplicitImpl;
+            }
+        }


Do we need this? Is calling this.IsAsyncCallConv different from this.Signature.IsAsyncCallConv?

MichalStrehovsky · 2025-11-04T08:00:50Z

src/coreclr/tools/Common/TypeSystem/Ecma/EcmaMethod.cs

+                    InitializeSignature();
+


This is a deoptimization, we should undo this.

(The return InitializeSignature(); form is tail-callable so JIT avoids building a frame for the method. The form in this PR is no longer tail callable, we need to build a frame to be able to call the method and return back.)

MichalStrehovsky · 2025-11-04T08:05:32Z

src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/ReadyToRunILProvider.cs

+                if (method.IsAsync)
+                {
+                    // We should not be creating any AsyncMethodVariants yet.
+                    // This hasn't been implemented.
+                    Debug.Assert(method.AsyncMethodKind is AsyncMethodKind.RuntimeAsync or AsyncMethodKind.AsyncExplicitImpl);
+                    return null;
+                }


Do we compile any async method bodies with this in place?

No, we skip all async methods with this.

MichalStrehovsky · 2025-11-04T08:13:47Z

src/coreclr/tools/Common/TypeSystem/Common/MethodDelegator.Async.cs

+    /// </summary>
+    public abstract partial class MethodDelegator : MethodDesc
+    {
+       public abstract override AsyncMethodKind AsyncMethodKind { get; }


Suggested change

public abstract override AsyncMethodKind AsyncMethodKind { get; }

public override AsyncMethodKind AsyncMethodKind => _wrappedMethod.AsyncMethodKind;

VSadov · 2025-11-04T08:49:58Z

What considerations lead to turning EcmaMethod into the "impl" variant of async? I understand there are going to be warts no matter how we implement this, but on the surface EcmaMethod not having the signature that it has in the metadata will lead to a lot of warts. My instinct would be to keep the signature as-is.

I think the most "weird" part about async variants is that before when we had synthetic "siblings" we typically had synthetic signatures matched to synthetic IL, but with async we can have pairs where metadata signature and IL from metadata are cross-matched.

I think the signatures are more important as more scenarios deal with how methods look "outside" rather than with what is in their IL bodies. There are scenarios, like reflection, where async variants simply "do not exist".
From that point it does make more sense for EcmaMethod to have the same signature as in metadata. It would need to support scenarios where EcmaMethod is matched to IL that is not in metadata, but that is likely less "weird" than if we have it the other way.

jkotas · 2025-11-04T13:53:25Z

src/coreclr/tools/Common/TypeSystem/Common/AsyncMethodVariant.cs

+    /// <summary>
+    /// Either the AsyncMethodImplVariant or AsyncMethodThunkVariant of a method marked .IsAsync.
+    /// </summary>
+    public partial class AsyncMethodVariant : MethodDelegator, IJitHashableOnly


We should make this the same between R2R and NAOT

More context: #121295 (comment) and #121295 (comment)

I think single MethodDesc type is fine, but it does not sound like that it should be JIT-lifetime only.

jkotas · 2025-11-04T13:55:58Z

src/coreclr/tools/Common/TypeSystem/Common/AsyncMethodVariant.cs

+namespace Internal.TypeSystem
+{
+    /// <summary>
+    /// Either the AsyncMethodImplVariant or AsyncMethodThunkVariant of a method marked .IsAsync.


Suggested change

/// Either the AsyncMethodImplVariant or AsyncMethodThunkVariant of a method marked .IsAsync.

/// MethodDesc that represents async calling convention entrypoint of a Task-returning method.

jtschuster added 3 commits October 30, 2025 11:04

Implement GetAsyncOtherVariant on instantiated methods

f54a832

Formatting and cleanup

f072d09

jtschuster requested review from MichalStrehovsky, Copilot, davidwrighton, eduardo-vp and jkoritzinsky October 30, 2025 21:54

github-actions bot added the area-crossgen2-coreclr label Oct 30, 2025

dotnet-policy-service bot assigned jtschuster Oct 30, 2025

Copilot AI reviewed Oct 30, 2025

View reviewed changes

Copilot review feedback. ILVerify still fails

3ff1a3c

jtschuster requested a review from Copilot October 30, 2025 22:39

Copilot AI reviewed Oct 30, 2025

View reviewed changes

src/coreclr/tools/Common/TypeSystem/Ecma/EcmaMethod.cs Outdated Show resolved Hide resolved

src/coreclr/tools/Common/TypeSystem/Common/TaskReturningAsyncThunk.cs Outdated Show resolved Hide resolved

Separate methods into sorting, diagnostic, canon, and async source files

f22ed2d

jtschuster requested a review from Copilot October 31, 2025 00:03

Copilot AI reviewed Oct 31, 2025

View reviewed changes

src/coreclr/tools/Common/TypeSystem/Common/InstantiatedMethod.Async.cs Show resolved Hide resolved

Add InstantiatedMethod.Async.cs to TypeSystem project

c4523a0

jkotas requested changes Oct 31, 2025

View reviewed changes

jtschuster added 3 commits November 3, 2025 14:01

Use single ephemeral AsyncMethodVariant MethodDesc for async methods,…

442b6eb

… and flags elsewhere.

Delete old files

eee7c72

Add missing file

3c6854a

jtschuster changed the title ~~Use EcmaMethod as the async "impl" variant and create async "thunk" MethodDesc classes~~ Rename AsyncMethodDesc to AsyncMethodVariant, add "AsyncVariant" flag to MethodWithToken and MethodWithGCInfo Nov 4, 2025

jtschuster added 2 commits November 3, 2025 16:30

Remove dummy ecma method file

d574847

Preserve Async-ness in GetMethodDefinition

62b9652

jkotas reviewed Nov 4, 2025

View reviewed changes

MichalStrehovsky reviewed Nov 4, 2025

View reviewed changes

jkotas added the runtime-async label Nov 4, 2025

jkotas reviewed Nov 4, 2025

View reviewed changes

	public abstract override AsyncMethodKind AsyncMethodKind { get; }
	public override AsyncMethodKind AsyncMethodKind => _wrappedMethod.AsyncMethodKind;

	/// Either the AsyncMethodImplVariant or AsyncMethodThunkVariant of a method marked .IsAsync.
	/// MethodDesc that represents async calling convention entrypoint of a Task-returning method.

Rename AsyncMethodDesc to AsyncMethodVariant, add "AsyncVariant" flag to MethodWithToken and MethodWithGCInfo #121218

Are you sure you want to change the base?

Rename AsyncMethodDesc to AsyncMethodVariant, add "AsyncVariant" flag to MethodWithToken and MethodWithGCInfo #121218

Conversation

jtschuster commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

MichalStrehovsky commented Oct 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

jtschuster commented Oct 30, 2025 •

edited

Loading

jkotas Oct 31, 2025 •

edited

Loading

MichalStrehovsky Nov 4, 2025 •

edited

Loading