JIT: fix invocation of unboxed entry when method returns struct #52998
Conversation
This file contains 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
If a value class method returns a struct, and its unboxed entry point requires a type context argument, make sure to pass the context argument properly. Also, fetch the type context (method table) from the box, rather than creating it from the class handle we have on hand; this tends to produce smaller code as we're often fetching the method table for other reasons anyways. Closes dotnet#52975.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
cc @dotnet/jit-contrib There are likely diffs in asp.net too, but I need to update the collection first. asm.libraries.pmi.windows.x64.checked Detail diffsasm.tests_libraries.pmi.windows.x64.checked Detail diffs |
|
ASP.NET diffs |
|
Kicked off the experimental CI group to do some PGO testing. That has a few known failures in OSR, so will have to look at the results closely. |
sandreenko
approved these changes
May 20, 2021
src/coreclr/jit/importer.cpp
Outdated
| @@ -21115,7 +21115,17 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, | |||
| // | |||
| if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) | |||
| { | |||
| call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); | |||
| // If there's a ret buf, the context is the second arg. | |||
There was a problem hiding this comment.
Nit:
Suggested change
| // If there's a ret buf, the context is the second arg. | |
| // If there's a ret buf, the method table is the second arg. |
src/coreclr/jit/importer.cpp
Outdated
| if (call->HasRetBufArg()) | ||
| { | ||
| GenTreeCall::Use* const beforeArg = call->gtCallArgs; | ||
| beforeArg->SetNext(gtNewCallArgs(methodTableArg)); |
There was a problem hiding this comment.
would gtInsertNewCallArgAfter(methodTableArg, call->gtCallArgs) work here? If I understand correctly 'gtCallArgspoints toretBufArgand we need to insertmethodTableArg` after it.
There was a problem hiding this comment.
Yes, that would work. You prefer it that way?
There was a problem hiding this comment.
yes, it would be similar to 'else' then I think.
src/coreclr/jit/importer.cpp
Outdated
| @@ -21194,26 +21204,26 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, | |||
| // locally, or was boxed locally but we were unable to remove the box for | |||
| // various reasons. | |||
| // | |||
| // We may still be able to update the call to invoke the unboxed entry. | |||
| // We can still to update the call to invoke the unboxed entry, if the | |||
There was a problem hiding this comment.
Nit:
Suggested change
| // We can still to update the call to invoke the unboxed entry, if the | |
| // We can still update the call to invoke the unboxed entry, if the |
src/coreclr/jit/importer.cpp
Outdated
| // | ||
| // Prepend for R2L arg passing or empty L2R passing | ||
| // Append for non-empty L2R | ||
| // | ||
| if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) | ||
| { | ||
| call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); | ||
| // If there's a ret buf, the context is the second arg. |
src/coreclr/jit/importer.cpp
Outdated
| if (call->HasRetBufArg()) | ||
| { | ||
| GenTreeCall::Use* const beforeArg = call->gtCallArgs; | ||
| beforeArg->SetNext(gtNewCallArgs(methodTableArg)); |
|
Linux arm release libraries: tests succeeded by failed to properly upload status: OSX failure is another instance of #52710. |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
If a value class method returns a struct, and its unboxed entry point
requires a type context argument, make sure to pass the context argument
properly.
Also, fetch the type context (method table) from the box, rather than
creating it from the class handle we have on hand; this tends to produce
smaller code as we're often fetching the method table for other reasons
anyways.
Closes #52975.