Skip to content
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

Fix fgCanTailCallViaJitHelper #70033

Merged
merged 3 commits into from
Jun 1, 2022
Merged

Fix fgCanTailCallViaJitHelper #70033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
badb6b2
Fix fgCanTailCallViaJitHelper
jakobbotsch May 31, 2022
f29a2db
Restore comment
jakobbotsch May 31, 2022
d642b1d
Fix lowering of vtable tailcalls via jit-helper
jakobbotsch May 31, 2022
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
20 changes: 15 additions & 5 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4935,11 +4935,21 @@ GenTree* Lowering::LowerVirtualVtableCall(GenTreeCall* call)
{
noway_assert(call->gtCallType == CT_USER_FUNC);

assert(call->gtArgs.HasThisPointer());
CallArg* thisArg = call->gtArgs.GetThisArg();
GenTree* thisArgNode;
if (call->IsTailCallViaJitHelper())
{
assert(call->gtArgs.CountArgs() > 0);
thisArgNode = call->gtArgs.GetArgByIndex(0)->GetNode();
}
else
{
assert(call->gtArgs.HasThisPointer());
thisArgNode = call->gtArgs.GetThisArg()->GetNode();
}

// get a reference to the thisPtr being passed
assert(thisArg->GetNode()->OperIs(GT_PUTARG_REG));
GenTree* thisPtr = thisArg->GetNode()->AsUnOp()->gtGetOp1();
assert(thisArgNode->OperIs(GT_PUTARG_REG));
GenTree* thisPtr = thisArgNode->AsUnOp()->gtGetOp1();

// If what we are passing as the thisptr is not already a local, make a new local to place it in
// because we will be creating expressions based on it.
Expand All @@ -4956,7 +4966,7 @@ GenTree* Lowering::LowerVirtualVtableCall(GenTreeCall* call)
vtableCallTemp = comp->lvaGrabTemp(true DEBUGARG("virtual vtable call"));
}

LIR::Use thisPtrUse(BlockRange(), &thisArg->GetNode()->AsUnOp()->gtOp1, thisArg->GetNode());
LIR::Use thisPtrUse(BlockRange(), &thisArgNode->AsUnOp()->gtOp1, thisArgNode);
ReplaceWithLclVar(thisPtrUse, vtableCallTemp);

lclNum = vtableCallTemp;
Expand Down
7 changes: 6 additions & 1 deletion src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17526,10 +17526,15 @@ bool Compiler::fgCheckStmtAfterTailCall()
//
bool Compiler::fgCanTailCallViaJitHelper()
{
#if !defined(TARGET_X86) || defined(UNIX_X86_ABI) || defined(FEATURE_READYTORUN)
Copy link
Member

Choose a reason for hiding this comment

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

did it basically turn helper-based tailcalls off for jit?

Copy link
Member

Choose a reason for hiding this comment

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

If so I guess we need more (if we have any) microbenchmarks written in F#

Copy link
Member Author

Choose a reason for hiding this comment

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

(I thought I responded but can't see my comment..?)

did it basically turn helper-based tailcalls off for jit?

No, it just made x86 use the same mechanism we use on all other platforms.

If so I guess we need more (if we have any) microbenchmarks written in F#

That would be great, and yes, this would probably have shown up as a noticeable perf regression.

#if !defined(TARGET_X86) || defined(UNIX_X86_ABI)
// On anything except windows X86 we have no faster mechanism available.
return false;
#else
// For R2R make sure we go through portable mechanism that the 'EE' side
// will properly turn into a runtime JIT.
if (opts.IsReadyToRun())
return false;

// The JIT helper does not properly handle the case where localloc was used.
if (compLocallocUsed)
return false;
Expand Down