Share return value handling in trim analysis#101398
Merged
sbomer merged 7 commits intodotnet:mainfrom Apr 25, 2024
Merged
Conversation
ghost
added
the
needs-area-label
sbomer
commented
Apr 22, 2024
| [ExpectedWarning ("IL2075", "GetMethod")] | ||
| public static void Test () | ||
| { | ||
| new Derived ().GetType ().GetMethod ("Method"); |
Member
Author
There was a problem hiding this comment.
The analyzer handling of object.GetType was not setting a return value, so moving to the shared logic fixes this discrepancy.
jtschuster
approved these changes
Apr 22, 2024
src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
Outdated
Show resolved
Hide resolved
And add missing cases to the switch statement for the analyzer.
Member
Author
|
I missed a few cases for the intrinsics that don't have special analyzer handling. I added those cases, and also made a fix to avoid crashing Release builds of the analyzer in case we add more cases without analyzer handling in the future. |
The PInvoke logic and the CheckAndReportRequires logic should not be needed for this intrinsic, and the old shared HandleCallAction would go to the default case that sets the return value to Top and returns false. Now this instead returns true and lets the shared return value logic kick in.
Member
Author
|
@MichalStrehovsky I'd appreciate your eyes on the latest commit b898b27. |
This was referenced Apr 25, 2024
Closed
Closed
Closed
MichalStrehovsky
approved these changes
Apr 25, 2024
Member
MichalStrehovsky
left a comment
MichalStrehovsky
left a comment
There was a problem hiding this comment.
LGTM otherwise, thanks!
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
Outdated
Show resolved
Hide resolved
| IntrinsicId intrinsicId, | ||
| out MultiValue? methodReturnValue) | ||
| { | ||
| MultiValue? maybeMethodReturnValue = methodReturnValue = null; |
Member
There was a problem hiding this comment.
What's the difference between maybeMethodReturnValue and methodReturnValue (do we need both?)
Member
Author
There was a problem hiding this comment.
Some code paths assign to the out param methodReturnValue and return immediately, but the paths that use AddReturnValue needed a separate local because local functions can't assign to out parameters of the containing function.
- Return instead of out param in HandleCall - FIx formatting
matouskozak
pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
This shares more of the HandleCall logic across ILLink/ILC/ILLink.RoslynAnalyzer. The return value handling has been moved from each tool's implementation (ReflectionMethodBodyScanner/TrimAnalysisVisitor) into the shared HandleCallAction, and the extra handling in MethodBodyScanner has been removed. * Fix Array_CreateInstance case The PInvoke logic and the CheckAndReportRequires logic should not be needed for this intrinsic, and the old shared HandleCallAction would go to the default case that sets the return value to Top and returns false. Now this instead returns true and lets the shared return value logic kick in.
michaelgsharp
pushed a commit
to michaelgsharp/runtime
that referenced
this pull request
May 9, 2024
This shares more of the HandleCall logic across ILLink/ILC/ILLink.RoslynAnalyzer. The return value handling has been moved from each tool's implementation (ReflectionMethodBodyScanner/TrimAnalysisVisitor) into the shared HandleCallAction, and the extra handling in MethodBodyScanner has been removed. * Fix Array_CreateInstance case The PInvoke logic and the CheckAndReportRequires logic should not be needed for this intrinsic, and the old shared HandleCallAction would go to the default case that sets the return value to Top and returns false. Now this instead returns true and lets the shared return value logic kick in.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
This shares more of the HandleCall logic across ILLink/ILC/ILLink.RoslynAnalyzer. The return value handling has been moved from each tool's implementation (ReflectionMethodBodyScanner/TrimAnalysisVisitor) into the shared HandleCallAction, and the extra handling in MethodBodyScanner has been removed.
Addresses the suggestions from #101212 (comment) and #101212 (comment).