Make IL Scanner report both variants#128107
Open
eduardo-vp wants to merge 1 commit into
Open
Conversation
Contributor
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the NativeAOT IL scanner’s call dependency tracking for runtime-async “task await” patterns so it no longer reports only the async-variant method, but instead reports both the original method and its async variant, allowing the JIT to select either implementation during compilation.
Changes:
- Stop rewriting the scanned call target to the async variant; instead, keep the original call target and track the async variant in parallel.
- Add dependency reporting for the async variant alongside the original call target, including generic-lookup-related nodes where applicable.
Comment on lines
+820
to
+826
| if (asyncVariantMethod != null && _canonMethod.IsSharedByGenericInstantiations) | ||
| { | ||
| MethodDesc avTargetMethod = asyncVariantMethod.GetCanonMethodTarget(CanonicalFormKind.Specific); | ||
| if (avTargetMethod.RequiresInstMethodDescArg()) | ||
| _dependencies.Add(GetGenericLookupHelper(ReadyToRunHelperId.MethodDictionary, asyncVariantRuntimeDeterminedMethod), reason); | ||
| else if (avTargetMethod.RequiresInstMethodTableArg()) | ||
| _dependencies.Add(GetGenericLookupHelper(ReadyToRunHelperId.TypeHandle, asyncVariantRuntimeDeterminedMethod.OwningType), reason); |
Comment on lines
+480
to
+483
| // If this is the task await pattern, the JIT will first resolve the call to the | ||
| // async variant, then may switch back to the original if the async variant is just | ||
| // a thunk (for non-runtime-async methods). Report both variants as dependencies such | ||
| // that the JIT can pick either one. |
Comment on lines
+820
to
+826
| if (asyncVariantMethod != null && _canonMethod.IsSharedByGenericInstantiations) | ||
| { | ||
| MethodDesc avTargetMethod = asyncVariantMethod.GetCanonMethodTarget(CanonicalFormKind.Specific); | ||
| if (avTargetMethod.RequiresInstMethodDescArg()) | ||
| _dependencies.Add(GetGenericLookupHelper(ReadyToRunHelperId.MethodDictionary, asyncVariantRuntimeDeterminedMethod), reason); | ||
| else if (avTargetMethod.RequiresInstMethodTableArg()) | ||
| _dependencies.Add(GetGenericLookupHelper(ReadyToRunHelperId.TypeHandle, asyncVariantRuntimeDeterminedMethod.OwningType), reason); |
Member
There was a problem hiding this comment.
This is going to be really difficult to review for correctness. I wonder if we can instead split ImportCall into two methods:
private void ImportCall(ILOpcode opcode, int token);
private void ImportCall(ILOpcode opcode, MethodDesc method, MethodDesc runtimeDeterminedMethod)
and have the token based ImportCall call the other one, potentially twice.
Member
Author
There was a problem hiding this comment.
Ah yes this is much better, will do
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IL Scanner currently reports only the async variant inside a runtime async method. While the JIT initially chooses this variant too, it can switch to the task returning variant if the async variant is a thunk. Make IL Scanner report both variants such that the JIT can choose either one.
Fixes #127179.