From 42b685cfdf411f6cccb0650821e2a13921bae4b2 Mon Sep 17 00:00:00 2001 From: Egor Chesakov Date: Wed, 9 Dec 2020 20:40:28 -0800 Subject: [PATCH] Fix regression with tail.callvirt transformation to helper call (#45527) * Add regression test for https://github.com/dotnet/runtime/issues/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 --- src/coreclr/jit/morph.cpp | 164 +++++++++++------- .../JIT/Directed/tailcall/more_tailcalls.cs | 44 +++++ .../JIT/Directed/tailcall/more_tailcalls.il | 150 +++++++++++++--- .../JitBlue/Runtime_45250/Runtime_45250.il | 93 ++++++++++ .../Runtime_45250/Runtime_45250.ilproj | 10 ++ 5 files changed, 378 insertions(+), 83 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_45250/Runtime_45250.il create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_45250/Runtime_45250.ilproj diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index b00e370c166f5..bc69b8cdeced6 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -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; @@ -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 @@ -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; @@ -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); diff --git a/src/tests/JIT/Directed/tailcall/more_tailcalls.cs b/src/tests/JIT/Directed/tailcall/more_tailcalls.cs index 141933daca50d..b5f8e7aeb9391 100644 --- a/src/tests/JIT/Directed/tailcall/more_tailcalls.cs +++ b/src/tests/JIT/Directed/tailcall/more_tailcalls.cs @@ -191,6 +191,8 @@ void TestCalc(Func 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 @@ -730,6 +732,48 @@ private static int InstantiatingStub1Other(T c0, T c1, T c2, T c3, T c4, T c5 return IL.Return(); } } + + class GenericInstance + { + 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 CreateInstance() + { + counter++; + return new GenericInstance(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), nameof(GenericInstance.NumberOfInstances))); + return IL.Return(); + } } class Instance diff --git a/src/tests/JIT/Directed/tailcall/more_tailcalls.il b/src/tests/JIT/Directed/tailcall/more_tailcalls.il index f46fb3d748f0f..84be13bab3cbd 100644 --- a/src/tests/JIT/Directed/tailcall/more_tailcalls.il +++ b/src/tests/JIT/Directed/tailcall/more_tailcalls.il @@ -43,14 +43,14 @@ .ver 1:0:0:0 } .module more_tailcalls.dll -// MVID: {2945C9C4-EED0-49EA-8A87-27475136401B} +// MVID: {81332D24-430E-4E25-88E2-383124EB3597} .custom instance void [System.Runtime]System.Security.UnverifiableCodeAttribute::.ctor() = ( 01 00 00 00 ) .imagebase 0x00400000 .file alignment 0x00000200 .stackreserve 0x00100000 .subsystem 0x0003 // WINDOWS_CUI .corflags 0x00000001 // ILONLY -// Image base: 0x00000273609B0000 +// Image base: 0x000001F273BC0000 // =============== CLASS MEMBERS DECLARATION =================== @@ -258,6 +258,68 @@ .class private auto ansi Program extends [System.Runtime]System.Object { + .class auto ansi nested private beforefieldinit GenericInstance`1 + extends [System.Runtime]System.Object + { + .field private class Program/GenericInstanceFactory factory + .method public hidebysig specialname rtspecialname + instance void .ctor(class Program/GenericInstanceFactory factory) cil managed + { + // Code size 14 (0xe) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: ldarg.0 + IL_0007: ldarg.1 + IL_0008: stfld class Program/GenericInstanceFactory class Program/GenericInstance`1::factory + IL_000d: ret + } // end of method GenericInstance`1::.ctor + + .method public hidebysig newslot virtual + instance int32 NumberOfInstances() cil managed noinlining + { + // Code size 12 (0xc) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: ldfld class Program/GenericInstanceFactory class Program/GenericInstance`1::factory + IL_0006: ldfld int32 Program/GenericInstanceFactory::counter + IL_000b: ret + } // end of method GenericInstance`1::NumberOfInstances + + } // end of class GenericInstance`1 + + .class auto ansi nested private beforefieldinit GenericInstanceFactory + extends [System.Runtime]System.Object + { + .field public int32 counter + .method public hidebysig instance class Program/GenericInstance`1 + CreateInstance() cil managed noinlining + { + // Code size 21 (0x15) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: ldarg.0 + IL_0002: ldfld int32 Program/GenericInstanceFactory::counter + IL_0007: ldc.i4.1 + IL_0008: add + IL_0009: stfld int32 Program/GenericInstanceFactory::counter + IL_000e: ldarg.0 + IL_000f: newobj instance void class Program/GenericInstance`1::.ctor(class Program/GenericInstanceFactory) + IL_0014: ret + } // end of method GenericInstanceFactory::CreateInstance + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + // Code size 7 (0x7) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: ret + } // end of method GenericInstanceFactory::.ctor + + } // end of class GenericInstanceFactory + .class auto ansi sealed nested private beforefieldinit '<>c__DisplayClass7_0' extends [System.Runtime]System.Object { @@ -348,7 +410,7 @@ IL_0008: stfld class [System.Runtime]System.Func`3 class Program/'<>c__DisplayClass7_1`1'::f IL_000d: ldarg.0 IL_000e: ldloc.0 - IL_000f: ldftn instance !0 class Program/'<>c__DisplayClass7_1`1'::'
b__30'() + IL_000f: ldftn instance !0 class Program/'<>c__DisplayClass7_1`1'::'
b__31'() IL_0015: newobj instance void class [System.Runtime]System.Func`1::.ctor(object, native int) IL_001a: ldarg.2 @@ -572,7 +634,7 @@ } // end of method '<>c__DisplayClass7_1`1'::.ctor .method assembly hidebysig instance !T - '
b__30'() cil managed + '
b__31'() cil managed { // Code size 18 (0x12) .maxstack 8 @@ -583,7 +645,7 @@ IL_000c: callvirt instance !2 class [System.Runtime]System.Func`3::Invoke(!0, !1) IL_0011: ret - } // end of method '<>c__DisplayClass7_1`1'::'
b__30' + } // end of method '<>c__DisplayClass7_1`1'::'
b__31' } // end of class '<>c__DisplayClass7_1`1' @@ -608,6 +670,7 @@ .field public static class [System.Runtime]System.Func`1 '<>9__7_16' .field public static class [System.Runtime]System.Func`1 '<>9__7_17' .field public static class [System.Runtime]System.Func`1 '<>9__7_18' + .field public static class [System.Runtime]System.Func`1 '<>9__7_30' .method private hidebysig specialname rtspecialname static void .cctor() cil managed { @@ -821,6 +884,15 @@ IL_000f: ret } // end of method '<>c'::'
b__7_18' + .method assembly hidebysig instance int32 + '
b__7_30'() cil managed + { + // Code size 6 (0x6) + .maxstack 8 + IL_0000: call int32 Program::VirtCallThisHasSideEffects() + IL_0005: ret + } // end of method '<>c'::'
b__7_30' + } // end of class '<>c' .field private static initonly native int s_calcStaticCalli @@ -878,7 +950,7 @@ Main() cil managed { .entrypoint - // Code size 1792 (0x700) + // Code size 1835 (0x72b) .maxstack 4 .locals init (class Program/'<>c__DisplayClass7_0' V_0, int32 V_1, @@ -1571,24 +1643,41 @@ !!0, string) IL_06d5: ldloc.0 - IL_06d6: ldfld bool Program/'<>c__DisplayClass7_0'::result - IL_06db: brfalse.s IL_06e9 + IL_06d6: ldsfld class [System.Runtime]System.Func`1 Program/'<>c'::'<>9__7_30' + IL_06db: dup + IL_06dc: brtrue.s IL_06f5 + + IL_06de: pop + IL_06df: ldsfld class Program/'<>c' Program/'<>c'::'<>9' + IL_06e4: ldftn instance int32 Program/'<>c'::'
b__7_30'() + IL_06ea: newobj instance void class [System.Runtime]System.Func`1::.ctor(object, + native int) + IL_06ef: dup + IL_06f0: stsfld class [System.Runtime]System.Func`1 Program/'<>c'::'<>9__7_30' + IL_06f5: ldc.i4.1 + IL_06f6: ldstr "Virtual call where computing \"this\" has side effects" + IL_06fb: callvirt instance void Program/'<>c__DisplayClass7_0'::'
g__Test|0'(class [System.Runtime]System.Func`1, + !!0, + string) + IL_0700: ldloc.0 + IL_0701: ldfld bool Program/'<>c__DisplayClass7_0'::result + IL_0706: brfalse.s IL_0714 - IL_06dd: ldstr "All tailcall-via-help succeeded" - IL_06e2: call void [System.Console]System.Console::WriteLine(string) - IL_06e7: br.s IL_06f3 + IL_0708: ldstr "All tailcall-via-help succeeded" + IL_070d: call void [System.Console]System.Console::WriteLine(string) + IL_0712: br.s IL_071e - IL_06e9: ldstr "One or more failures in tailcall-via-help test" - IL_06ee: call void [System.Console]System.Console::WriteLine(string) - IL_06f3: ldloc.0 - IL_06f4: ldfld bool Program/'<>c__DisplayClass7_0'::result - IL_06f9: brtrue.s IL_06fd + IL_0714: ldstr "One or more failures in tailcall-via-help test" + IL_0719: call void [System.Console]System.Console::WriteLine(string) + IL_071e: ldloc.0 + IL_071f: ldfld bool Program/'<>c__DisplayClass7_0'::result + IL_0724: brtrue.s IL_0728 - IL_06fb: ldc.i4.1 - IL_06fc: ret + IL_0726: ldc.i4.1 + IL_0727: ret - IL_06fd: ldc.i4.s 100 - IL_06ff: ret + IL_0728: ldc.i4.s 100 + IL_072a: ret } // end of method Program::Main .method public hidebysig static void Calc(int32& x, @@ -2751,6 +2840,21 @@ IL_001e: ret } // end of method Program::InstantiatingStub1Other + .method private hidebysig static int32 + VirtCallThisHasSideEffects() cil managed noinlining + { + // Code size 26 (0x1a) + .maxstack 1 + IL_0000: ldc.i4 0x3e8 + IL_0005: localloc + IL_0007: pop + IL_0008: newobj instance void Program/GenericInstanceFactory::.ctor() + IL_000d: call instance class Program/GenericInstance`1 Program/GenericInstanceFactory::CreateInstance() + IL_0012: tail. + IL_0014: callvirt instance int32 class Program/GenericInstance`1::NumberOfInstances() + IL_0019: ret + } // end of method Program::VirtCallThisHasSideEffects + .method public hidebysig specialname rtspecialname instance void .ctor() cil managed { @@ -3526,7 +3630,8 @@ IL_0006: ret } // end of method ClassImpl::.ctor - .property instance class BaseClass Other() + .property instance callconv(8) class BaseClass + Other() { .get instance class BaseClass ClassImpl::get_Other() .set instance void ClassImpl::set_Other(class BaseClass) @@ -3754,7 +3859,8 @@ IL_0006: ret } // end of method InterfaceImpl::.ctor - .property instance class IInterface Other() + .property instance callconv(8) class IInterface + Other() { .get instance class IInterface InterfaceImpl::get_Other() .set instance void InterfaceImpl::set_Other(class IInterface) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_45250/Runtime_45250.il b/src/tests/JIT/Regression/JitBlue/Runtime_45250/Runtime_45250.il new file mode 100644 index 0000000000000..88cb0a35d339c --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_45250/Runtime_45250.il @@ -0,0 +1,93 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +.assembly extern System.Runtime +{ +} +.assembly Runtime_45250 +{ +} +.module Runtime_45250.dll + +.class private auto ansi beforefieldinit Runtime_45250.Program + extends [System.Runtime]System.Object +{ + .class auto ansi nested private beforefieldinit Func`1 + extends [System.Runtime]System.Object + { + .method public hidebysig newslot virtual + instance void Run() cil managed noinlining + { + .maxstack 8 + IL_0000: ret + } + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: ret + } + } + + .class auto ansi nested private beforefieldinit FuncGetter + extends [System.Runtime]System.Object + { + .field public int32 counter + + .method public hidebysig instance class Runtime_45250.Program/Func`1 + Get() cil managed noinlining + { + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: ldarg.0 + IL_0002: ldfld int32 Runtime_45250.Program/FuncGetter::counter + IL_0007: ldc.i4.1 + IL_0008: add + IL_0009: stfld int32 Runtime_45250.Program/FuncGetter::counter + IL_000e: newobj instance void class Runtime_45250.Program/Func`1::.ctor() + IL_0013: ret + } + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: ret + } + } + + .method private hidebysig static void Run(class Runtime_45250.Program/FuncGetter funcGetter) cil managed noinlining + { + .maxstack 8 + IL_0000: ldc.i4 45250 + IL_0002: localloc + IL_0004: pop + IL_0005: ldarg.0 + IL_0006: call instance class Runtime_45250.Program/Func`1 Runtime_45250.Program/FuncGetter::Get() + IL_000b: tail. + IL_000d: callvirt instance void class Runtime_45250.Program/Func`1::Run() + IL_0012: ret + } + + .method private hidebysig static int32 + Main(string[] args) cil managed + { + .entrypoint + .maxstack 8 + IL_0000: newobj instance void Runtime_45250.Program/FuncGetter::.ctor() + IL_0005: dup + IL_0006: call void Runtime_45250.Program::Run(class Runtime_45250.Program/FuncGetter) + IL_000b: ldfld int32 Runtime_45250.Program/FuncGetter::counter + IL_0010: ldc.i4.1 + IL_0011: beq.s IL_0015 + IL_0013: ldc.i4.1 + IL_0014: ret + IL_0015: ldc.i4.s 100 + IL_0017: ret + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_45250/Runtime_45250.ilproj b/src/tests/JIT/Regression/JitBlue/Runtime_45250/Runtime_45250.ilproj new file mode 100644 index 0000000000000..7dab57fe6d225 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_45250/Runtime_45250.ilproj @@ -0,0 +1,10 @@ + + + Exe + None + True + + + + +