Skip to content

Commit

Permalink
Fix regression with tail.callvirt transformation to helper call (#45527)
Browse files Browse the repository at this point in the history 
* Add regression test for #45250

* Add VirtCallThisHasSideEffects to more_tailcalls.cs and update more_tailcalls.il

* Spill "this" if needed to avoid evaluating it twice when "this" is used to compute the target function pointer in morph.cpp
  • Loading branch information
@echesakov
echesakov authored Dec 10, 2020
1 parent 663d431 commit 42b685c
Show file tree
Hide file tree
Showing 5 changed files with 378 additions and 83 deletions.
164 changes: 103 additions & 61 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7867,9 +7867,102 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL
call->fgArgInfo = nullptr;
}

const bool stubNeedsTargetFnPtr = (help.flags & CORINFO_TAILCALL_STORE_TARGET) != 0;

GenTree* doBeforeStoreArgsStub = nullptr;
GenTree* thisPtrStubArg = nullptr;

// Put 'this' in normal param list
if (call->gtCallThisArg != nullptr)
{
JITDUMP("Moving this pointer into arg list\n");
GenTree* objp = call->gtCallThisArg->GetNode();
GenTree* thisPtr = nullptr;
call->gtCallThisArg = nullptr;

// JIT will need one or two copies of "this" in the following cases:
// 1) the call needs null check;
// 2) StoreArgs stub needs the target function pointer address and if the call is virtual
// the stub also needs "this" in order to evalute the target.

const bool callNeedsNullCheck = call->NeedsNullCheck();
const bool stubNeedsThisPtr = stubNeedsTargetFnPtr && virtualCall;

// TODO-Review: The following transformation is implemented under assumption that
// both conditions can be true. However, I could not construct such example
// where a virtual tail call would require null check. In case, if the conditions
// are mutually exclusive the following could be simplified.

if (callNeedsNullCheck || stubNeedsThisPtr)
{
// Clone "this" if "this" has no side effects.
if ((objp->gtFlags & GTF_SIDE_EFFECT) == 0)
{
thisPtr = gtClone(objp, true);
}

// Create a temp and spill "this" to the temp if "this" has side effects or "this" was too complex to clone.
if (thisPtr == nullptr)
{
const unsigned lclNum = lvaGrabTemp(true DEBUGARG("tail call thisptr"));

// tmp = "this"
doBeforeStoreArgsStub = gtNewTempAssign(lclNum, objp);

if (callNeedsNullCheck)
{
// COMMA(tmp = "this", deref(tmp))
GenTree* tmp = gtNewLclvNode(lclNum, objp->TypeGet());
GenTree* nullcheck = gtNewNullCheck(tmp, compCurBB);
doBeforeStoreArgsStub = gtNewOperNode(GT_COMMA, TYP_VOID, doBeforeStoreArgsStub, nullcheck);
}

thisPtr = gtNewLclvNode(lclNum, objp->TypeGet());

if (stubNeedsThisPtr)
{
thisPtrStubArg = gtNewLclvNode(lclNum, objp->TypeGet());
}
}
else
{
if (callNeedsNullCheck)
{
// deref("this")
doBeforeStoreArgsStub = gtNewNullCheck(objp, compCurBB);

if (stubNeedsThisPtr)
{
thisPtrStubArg = gtClone(objp, true);
}
}
else
{
assert(stubNeedsThisPtr);

thisPtrStubArg = objp;
}
}

call->gtFlags &= ~GTF_CALL_NULLCHECK;

assert((thisPtrStubArg != nullptr) == stubNeedsThisPtr);
}
else
{
thisPtr = objp;
}

// During rationalization tmp="this" and null check will be materialized
// in the right execution order.
assert(thisPtr != nullptr);
call->gtCallArgs = gtPrependNewCallArg(thisPtr, call->gtCallArgs);
call->fgArgInfo = nullptr;
}

// We may need to pass the target, for instance for calli or generic methods
// where we pass instantiating stub.
if ((help.flags & CORINFO_TAILCALL_STORE_TARGET) != 0)
if (stubNeedsTargetFnPtr)
{
JITDUMP("Adding target since VM requested it\n");
GenTree* target;
Expand Down Expand Up @@ -7912,11 +8005,7 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL
}

eeGetCallInfo(call->tailCallInfo->GetToken(), nullptr, (CORINFO_CALLINFO_FLAGS)flags, &callInfo);

assert(call->gtCallThisArg != nullptr);
// TODO: Proper cloning of the this pointer.
target = getVirtMethodPointerTree(gtCloneExpr(call->gtCallThisArg->GetNode()),
call->tailCallInfo->GetToken(), &callInfo);
target = getVirtMethodPointerTree(thisPtrStubArg, call->tailCallInfo->GetToken(), &callInfo);
}

// Insert target as last arg
Expand All @@ -7931,60 +8020,6 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL
call->fgArgInfo = nullptr;
}

// Put 'this' in normal param list
if (call->gtCallThisArg != nullptr)
{
JITDUMP("Moving this pointer into arg list\n");
GenTree* thisPtr = nullptr;
GenTree* objp = call->gtCallThisArg->GetNode();
call->gtCallThisArg = nullptr;

if (call->NeedsNullCheck())
{
// clone "this" if "this" has no side effects.
if ((objp->gtFlags & GTF_SIDE_EFFECT) == 0)
{
thisPtr = gtClone(objp, true);
}

var_types vt = objp->TypeGet();
if (thisPtr == nullptr)
{
// create a temp if either "this" has side effects or "this" is too complex to clone.

// tmp = "this"
unsigned lclNum = lvaGrabTemp(true DEBUGARG("tail call thisptr"));
GenTree* asg = gtNewTempAssign(lclNum, objp);

// COMMA(tmp = "this", deref(tmp))
GenTree* tmp = gtNewLclvNode(lclNum, vt);
GenTree* nullcheck = gtNewNullCheck(tmp, compCurBB);
asg = gtNewOperNode(GT_COMMA, TYP_VOID, asg, nullcheck);

// COMMA(COMMA(tmp = "this", deref(tmp)), tmp)
thisPtr = gtNewOperNode(GT_COMMA, vt, asg, gtNewLclvNode(lclNum, vt));
}
else
{
// thisPtr = COMMA(deref("this"), "this")
GenTree* nullcheck = gtNewNullCheck(thisPtr, compCurBB);
thisPtr = gtNewOperNode(GT_COMMA, vt, nullcheck, gtClone(objp, true));
}

call->gtFlags &= ~GTF_CALL_NULLCHECK;
}
else
{
thisPtr = objp;
}

// During rationalization tmp="this" and null check will be materialized
// in the right execution order.
assert(thisPtr != nullptr);
call->gtCallArgs = gtPrependNewCallArg(thisPtr, call->gtCallArgs);
call->fgArgInfo = nullptr;
}

// This is now a direct call to the store args stub and not a tailcall.
call->gtCallType = CT_USER_FUNC;
call->gtCallMethHnd = help.hStoreArgs;
Expand All @@ -7996,8 +8031,15 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL
call->gtType = TYP_VOID;
call->gtReturnType = TYP_VOID;

GenTree* callStoreArgsStub = call;

if (doBeforeStoreArgsStub != nullptr)
{
callStoreArgsStub = gtNewOperNode(GT_COMMA, TYP_VOID, doBeforeStoreArgsStub, callStoreArgsStub);
}

GenTree* finalTree =
gtNewOperNode(GT_COMMA, callDispatcherAndGetResult->TypeGet(), call, callDispatcherAndGetResult);
gtNewOperNode(GT_COMMA, callDispatcherAndGetResult->TypeGet(), callStoreArgsStub, callDispatcherAndGetResult);

finalTree = fgMorphTree(finalTree);

Expand Down
44 changes: 44 additions & 0 deletions src/tests/JIT/Directed/tailcall/more_tailcalls.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,8 @@ void TestCalc<T>(Func<int, int, T> f, T expected, string name)
a[99] = 1;
Test(() => InstantiatingStub1(0, 0, "string", a), a.Length + 1, "Instantiating stub direct");

Test(() => VirtCallThisHasSideEffects(), 1, "Virtual call where computing \"this\" has side effects");

if (result)
Console.WriteLine("All tailcall-via-help succeeded");
else
Expand Down Expand Up @@ -730,6 +732,48 @@ private static int InstantiatingStub1Other<T>(T c0, T c1, T c2, T c3, T c4, T c5
return IL.Return<int>();
}
}

class GenericInstance<T>
{
private GenericInstanceFactory factory;

public GenericInstance(GenericInstanceFactory factory)
{
this.factory = factory;
}

[MethodImpl(MethodImplOptions.NoInlining)]
public virtual int NumberOfInstances()
{
return factory.counter;
}
}

class GenericInstanceFactory
{
public int counter = 0;

[MethodImpl(MethodImplOptions.NoInlining)]
public GenericInstance<string> CreateInstance()
{
counter++;
return new GenericInstance<string>(this);
}
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static int VirtCallThisHasSideEffects()
{
IL.Push(1000);
IL.Emit.Localloc();
IL.Emit.Pop();
GenericInstanceFactory fact = new GenericInstanceFactory();
IL.Push(fact);
IL.Emit.Call(new MethodRef(typeof(GenericInstanceFactory), nameof(GenericInstanceFactory.CreateInstance)));
IL.Emit.Tail();
IL.Emit.Callvirt(new MethodRef(typeof(GenericInstance<string>), nameof(GenericInstance<string>.NumberOfInstances)));
return IL.Return<int>();
}
}

class Instance
Expand Down
Loading

0 comments on commit 42b685c

Please sign in to comment.