Move async variant resolution into getCallInfo#124550
Move async variant resolution into getCallInfo#124550jakobbotsch wants to merge 8 commits intodotnet:mainfrom
getCallInfo#124550Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This pull request refactors async variant resolution by moving it from resolveToken into getCallInfo. This change fixes issue #124545 where static virtual calls were incorrectly going through task-returning thunks instead of calling the async implementation directly.
Changes:
- Removes
CORINFO_TOKENKIND_AwaitandCORINFO_TOKENKIND_AwaitVirtualtoken kinds from the JIT/EE interface - Adds
CORINFO_CALLINFO_ALLOWASYNCVARIANTflag to request async variant resolution duringgetCallInfo - Moves async variant resolution logic from token resolution to call info resolution, ensuring it happens after direct call determination
- Updates JIT code to pass the new flag when async patterns are detected
- Replaces the
combinehelper function with proper C++ operators for flag manipulation
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/inc/corinfo.h | Removes Await token kinds, adds ALLOWASYNCVARIANT flag, adds resolvedAsyncVariant field to CORINFO_CALL_INFO |
| src/coreclr/inc/jiteeversionguid.h | Updates JIT/EE interface GUID for the interface change |
| src/coreclr/vm/jitinterface.cpp | Removes async variant resolution from resolveToken, adds it to getCallInfo after directCall determination |
| src/coreclr/jit/importer.cpp | Updates call import logic to use new flag instead of special token kinds |
| src/coreclr/jit/importercalls.cpp | Removes obsolete comment about CEE_CALLI async handling |
| src/coreclr/jit/morph.cpp | Updates flag combining to use new operators |
| src/coreclr/jit/ee_il_dll.hpp | Replaces combine function with proper C++ operators for CORINFO_CALLINFO_FLAGS |
| private MethodDesc GetRuntimeDeterminedMethodForTokenWithTemplate(ref CORINFO_RESOLVED_TOKEN pResolvedToken, MethodDesc templateMethod) | ||
| { | ||
| object result = GetRuntimeDeterminedObjectForToken(ref pResolvedToken); | ||
| Debug.Assert(result is MethodDesc); | ||
|
|
||
| if (templateMethod.IsAsyncVariant() && !((MethodDesc)result).IsAsyncVariant()) | ||
| { | ||
| result = _compilation.TypeSystemContext.GetAsyncVariantMethod((MethodDesc)result); | ||
| } | ||
|
|
||
| return (MethodDesc)result; | ||
| } |
There was a problem hiding this comment.
Not so sure about this. I am open to suggestions of alternatives. This is the replacement of the CorInfoTokenKind.CORINFO_TOKENKIND_Await that used to be in GetRuntimeDeterminedObjectForToken.
There was a problem hiding this comment.
How about private object GetRuntimeDeterminedObjectForToken(ref CORINFO_RESOLVED_TOKEN pResolvedToken, bool isAsyncVariant = false) (add optional parameter to the existing method)?
The value of the bool is easy to pass in getCallInfo (we can just remember it) and elsewhere we populate it from whatever was considered the template.
There was a problem hiding this comment.
Implemented that. Can you take another look?
| if ((flags & CORINFO_CALLINFO_ALLOWASYNCVARIANT) && pMD->ReturnsTaskOrValueTask()) | ||
| { | ||
| if (!directCall || pTargetMD->IsAsyncThunkMethod()) | ||
| { | ||
| // Either a virtual call or a direct call where the async variant |
There was a problem hiding this comment.
getCallInfo now resolves async variants when CORINFO_CALLINFO_ALLOWASYNCVARIANT is set, but (unlike the NativeAOT implementation) it doesn’t exclude delegate types. For Delegate.Invoke calls, resolving to an async variant can defeat the existing wrapperDelegateInvoke fast-path and may introduce unnecessary thunking. Consider adding a guard like !pMD->GetMethodTable()->IsDelegate() (or equivalent) before calling GetAsyncVariant.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Green CI including nativeaot-outerloop! But crossgen2 handling is missing so will push that. |
MichalStrehovsky
left a comment
There was a problem hiding this comment.
The managed part LGTM otherwise!
| result = ((TypeDesc)result).MakeArrayType(); | ||
|
|
||
| if (pResolvedToken.tokenType is CorInfoTokenKind.CORINFO_TOKENKIND_Await or CorInfoTokenKind.CORINFO_TOKENKIND_AwaitVirtual) | ||
| if (isAsyncVariant && !((MethodDesc)result).IsAsyncVariant()) |
There was a problem hiding this comment.
Is this part necessary?
| if (isAsyncVariant && !((MethodDesc)result).IsAsyncVariant()) | |
| if (isAsyncVariant) |
There was a problem hiding this comment.
Yes, it is needed since the token may represent async variants directly for internal compiler-generated IL (like the thunks), so the result may already be the async variant for those cases. GetAsyncVariant asserts since they aren't task-returning in those cases.
(We won't hit that from the getCallInfo callers, but we can hit it from uses in other functions)
| else if (opcode == CEE_CALLI) | ||
| { | ||
| // Used for unboxing/instantiating stubs | ||
| JITDUMP("Call is an async calli\n"); | ||
| } |
There was a problem hiding this comment.
Unrelated cleanup, this is no longer needed.
|
I think @VSadov is OOF. @jkotas can you take a look at the VM bits? |
| // async variant or NULL if no async variant was resolved. | ||
| // This is the async variant of the token's method and differs from hMethod | ||
| // of this class in cases of sharing, constrained resolution etc. | ||
| CORINFO_METHOD_HANDLE resolvedAsyncVariant; |
There was a problem hiding this comment.
Why is this a separate CORINFO_METHOD_HANDLE field and not just returned in hMethod?
The idea with getCallInfo is that it tells the JIT what to call exactly. I assume that if the VM tells the JIT to call async variant, the JIT is better to call it. Is that correct?
There was a problem hiding this comment.
hMethod does return the async variant of the target method, and that's what the JIT calls, but that differs from the async variant of the token's method in cases of sharing or due to constrained calls. The JIT takes this CORINFO_METHOD_HANDLE and updates hMethod of the token with it since the token itself is passed back into the EE in various cases. That makes token match what was happening before when it was part of resolveToken (specifically so that the EE sees the same hMethod in that token).
I am not 100% sure what the EE sides end up using hMethod of the token for in those callbacks and whether this could be simplified or not.
There was a problem hiding this comment.
I suppose the primary one is that we call embedGenericHandle and pass it the token to compute the runtime lookup for an instantiation parameter, here:
runtime/src/coreclr/jit/importercalls.cpp
Lines 805 to 810 in 4eebedb
This works now because the JIT updates the token's hMethod with callInfo.resolvedAsyncVariant when it notices that the async variant was resolved.
Perhaps a cleaner way would be if getCallInfo directly returned the runtime lookup instead of this embed roundtrip. The current token vs callInfo.hMethod mismatch leads to other problems here, I guess this comment is referencing the same thing:
runtime/src/coreclr/vm/jitinterface.cpp
Lines 5252 to 5269 in 4eebedb
There was a problem hiding this comment.
How about we introduce CORINFO_TOKENKIND_Async, then delete CORINFO_METHOD_HANDLE resolvedAsyncVariant;, and keep using hMethod, but also set the async tokenType on the EE side whenever we return the async variant.
That way we'll keep clearly marking the token as "async variant token". It is weird that the EE side would change tokenType, but also JIT side changing hMethod is not exactly clean.
There was a problem hiding this comment.
It would work but it will be a new contract for getCallInfo (now has another output, requires SPMI changes).
It's probably a bit better but I am leaning towards making it so that we do not need the token anymore for non-trivial things after getCallInfo has been called by making it return the runtime lookup. It seems it is the only way to make things self-consistent.
CORINFO_TOKENKINDsCORINFO_CALLINFO_ALLOWASYNCVARIANTflag and pass this forgetCallInfogetCallInfoto resolve async variantsgetCallInfoso that we can know if an await is direct or not at the time where we resolve async variantsAnother benefit is that we now automatically take
IsEffectivelySealedinto account in NAOT.Fix #124545