-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: save generics context for late devirtualization #63420
Conversation
If we have a non-inline candidate call with generics context, save the context so it's available for late devirtualization. Fixes a missing devirtualization reported in dotnet#63283. I am deliberately leaving `LateDevirtualizationInfo` more general than necessary as it may serve as a jumping-off point for enabling late inlining.
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsIf we have a non-inline candidate call with generics context, save Fixes a missing devirtualization reported in #63283. I am deliberately leaving
|
cc @dotnet/jit-contrib SPMI diffs not interesting as any method that would benefit from new behavior gets a missing info error (~176K of them all told). PMI diffs show a few hundred improvements. Regressions are from extra CSEs/spills.
Codegen for
|
Seeing some failures where |
Updated to copy the union field --we can clone We can't safely clone inline or gdv candidates this way, but we don't do that now. |
@EgorBo can you take a look? |
{ | ||
JITDUMP("\nSaving context %p for call [%06u]\n", exactContextHnd, dspTreeID(origCall)); | ||
origCall->gtCallMoreFlags |= GTF_CALL_M_LATE_DEVIRT; | ||
LateDevirtualizationInfo* const info = new (this, CMK_Inlining) LateDevirtualizationInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning to add more fields to LateDevirtualizationInfo
? because for now it looks like allocation is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? That's what I meant by
I am deliberately leaving
LateDevirtualizationInfo
more general than
necessary as it may serve as a jumping-off point for enabling late inlining.
src/coreclr/jit/fginline.cpp
Outdated
const bool isLateDevirtualization = true; | ||
bool explicitTailCall = (call->AsCall()->gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; | ||
const bool explicitTailCall = (call->AsCall()->gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a nit: there is IsTailPrefixedCall()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, will change it over. Also, we already know call
is GenTreeCall*
.
Improvement on win-arm64 dotnet/perf-autofiling-issues#2978 |
If we have a non-inline candidate call with generics context, save
the context so it's available for late devirtualization.
Fixes a missing devirtualization reported in #63283.
I am deliberately leaving
LateDevirtualizationInfo
more general thannecessary as it may serve as a jumping-off point for enabling late
inlining.