Skip to content

JIT: work around issues invoking the unboxed entry when prejitting #52605

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 12, 2021
Merged

JIT: work around issues invoking the unboxed entry when prejitting #52605

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21008,6 +21008,14 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
{
JITDUMP("Have a direct explicit tail call to boxed entry point; can't optimize further\n");
}
else if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT))
Copy link
Member

@MichalStrehovsky MichalStrehovsky May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not abort this in resolveVirtualMethod on the crossgen2? We could limit this to the cases when the devirtualized method body is shared. Do we want to give up on this optimizations for nongeneric valuetypes or unshared generic valuetypes?

Copy link
Member

@MichalStrehovsky MichalStrehovsky May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix would be something along the lines of adding

if (impl.GetCanonMethodTarget(CanonicalFormKind.Specific).IsCanonicalMethod(CanonicalFormKind.Any))
{
    return false;
}

to the "if (impl.OwningType.IsValueType)" block in resolveVirtualMethod.

Copy link
Member Author

@AndyAyersMS AndyAyersMS May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping whatever fix we take here is perhaps temporary.

So far the devirtualization hasn't lead to bad codegen -- just the follow-on attempt to update the call site to invoke the unboxed entry.

I suppose you could argue that we're lucky and the (nominal) unshared unboxing method and the shared unboxing method are really the same method and neither can be inlined, so currently nothing bad comes of confusing the two.

The runtime and crossgen1 should allow devirtualization here too. But currently that implementation of resolveVirtualMethod is blocked because we present it with a mixture of an exact implementation type and a shared interface method (and hence type) and this leads the runtime to deduce that the exact type does not implement the interface.

I find it fairly tricky to reason about correct behavior in these mixed shared/exact cases. We will see more and more of this as via PGO the jit is able to get a hold of exact types in many places where it would not have been possible before.

Copy link
Member

@MichalStrehovsky MichalStrehovsky May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just meant that if we happen to ship with this for .NET 6, it would be unfortunate to disable all of this if it works only because it doesn't work for KeyValuePair (there are very few other popular generic structs).

#51982 is an example of a devirtualization problem with shared code (this one affects all of the: VM, crossgen and crossgen2). There are various weird corner cases like this that we probably don't have tests for.

Copy link
Member Author

@AndyAyersMS AndyAyersMS May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far the devirtualization hasn't lead to bad codegen

Evidently this is not the case -- looking at #51982 and #51983 it looks like the problems are more widespread.

{
// Per https://github.com/dotnet/runtime/issues/52483, crossgen2 seemingly gets
// confused about whether the unboxed entry requires an extra arg.
// So defer further optimization for now.
//
JITDUMP("Have a direct boxed entry point, prejitting. Can't optimize further yet.\n");
}
else
{
JITDUMP("Have a direct call to boxed entry point. Trying to optimize to call an unboxed entry point\n");
Expand Down