Report CALLCONV_ASYNCCALL and FLAG_ASYNC on AsyncExplicitImpl methods #121403
Conversation
`Signature.IsAsyncCall` is not true for these. We could also fix this when we materialize the signature from the ECMA file, but I'm not sure we want to go there.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances async method handling in the JIT interface by ensuring that special async intrinsics (async methods that don't return Task or ValueTask) are correctly marked with the async calling convention, even though their signatures don't have the async bit set.
- Adds logic to set
CORINFO_CALLCONV_ASYNCCALLfor async intrinsics in signature info - Adds logic to set
CORJIT_FLAG_ASYNCfor async intrinsics in JIT flags - Compensates for special CoreLib async intrinsics that have
IsAsyncmetadata but don't return Task/ValueTask
Do we actually need The async calling convention is similar to the hidden argument in shared generic calling convention. We do not keep track of the hidden parameter in |
|
|
There are methods that are AsyncExplicitImpl and also not intrinsics. Like We reserve the right to have Async callconv methods that are not task-returning in the corelib. The need sometimes arises in the infrastructure code. We had more of such helpers originally, now we have less, but as we go into optimizing the managed infrastructure or its interfacing with Task/ValueTask, we may have more. As an example: The reasons why only in corelib is just because we do not want to make such methods a public-facing feature. The behavior is well defined and can be documented, and maybe even have some special support in Roslyn. As a matter of scoping public API we chose not to. I am just pointing that, while methods like |
This matches how |
In some sense everything is special-cased - based on a signature, on presence of generic parameters, on MethodImpl, ... These are not special-cased. They are just async methods. MethodImpl.Async says so. There is no requirement for the set of AsyncExplicitImpl to be known to the JIT by name and there is no requirement for them to be marked as Also adding/removing/renaming such methods was more laborious as JIT code would need to be changed in sync. Possibly would need to change R2R version every time, if we supported R2R - that is just for some internal implementation refactoring. |
|
@copilot replace uses of MethodSignature.IsAsyncCall with a new extension method |
|
@MichalStrehovsky I've opened a new pull request, #121420, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
Okay so "Mention copilot in a comment to make changes to this pull request." above is a lie. It's going to make a new pull request. |
New pull request against your branch |
…on method (#121420) Consolidates async calling convention detection logic into a single `MethodDesc` extension method, removing the need for `MethodSignatureFlags.AsyncCall` flag. **Changes:** - **Added `IsAsyncCall` extension method** on `MethodDesc` that unifies detection logic: returns true for async variants and async intrinsics that don't return Task/ValueTask - **Removed `MethodSignatureFlags.AsyncCall`** enum value and `MethodSignature.IsAsyncCall` property - **Updated call sites** in `CorInfoImpl.cs` to use the new extension method for `CORINFO_CALLCONV_ASYNCCALL` and `CORJIT_FLAG_ASYNC` - **Simplified `AsyncMethodVariant`** to no longer set the AsyncCall flag in signature **Rationale:** Async calling convention is now determined at the MethodDesc level (similar to hidden parameters in generic calling conventions) rather than being stored in signature flags. This properly handles both async variants (which have the flag set by signature transformation) and async intrinsics (which have `IsAsync` metadata but don't return Task/ValueTask). ```csharp // Before: Logic scattered across multiple locations if (method.IsAsync && !method.Signature.ReturnsTaskOrValueTask()) sig->callConv |= CorInfoCallConv.CORINFO_CALLCONV_ASYNCCALL; // After: Unified in extension method if (method.IsAsyncCall()) sig->callConv |= CorInfoCallConv.CORINFO_CALLCONV_ASYNCCALL; ``` <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: MichalStrehovsky <[email protected]>
|
/ba-g timeout on android |
Signature.IsAsyncCallis not true for these because we currently only set it on variants.We could also fix this when we materialize the signature from the ECMA file, but I'm not sure we want to go there.
Cc @dotnet/ilc-contrib