From 8c12e27d308a35132f8608c0e6f988ce9617c59a Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 3 Jan 2019 19:11:34 -0800 Subject: [PATCH 1/5] When performing devirtualization we can not do both an unboxing optimization and a tail call optimization Explicit tail calls are now checked for and blocked from performing an unboxing operation in impDevirtualizeCall If late devirtualization calls impDevirtualizeCall with an IMPLICIT_TAILCALL we will clear this flag if we decide to perform the unboxing operation. --- src/jit/compiler.h | 3 ++- src/jit/flowgraph.cpp | 3 ++- src/jit/importer.cpp | 25 ++++++++++++++++++++++--- src/jit/indirectcalltransformer.cpp | 6 ++++-- 4 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/jit/compiler.h b/src/jit/compiler.h index 1fc90679b4bf..5f42bb5a9ce4 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -3329,7 +3329,8 @@ class Compiler unsigned* methodFlags, CORINFO_CONTEXT_HANDLE* contextHandle, CORINFO_CONTEXT_HANDLE* exactContextHandle, - bool isLateDevirtualization); + bool isLateDevirtualization, + bool isTailCall); //========================================================================= // PROTECTED diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index 4e2fc40bba4e..feaceb3dcbfb 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -22510,7 +22510,8 @@ Compiler::fgWalkResult Compiler::fgLateDevirtualization(GenTree** pTree, fgWalkD unsigned methodFlags = 0; CORINFO_CONTEXT_HANDLE context = nullptr; const bool isLateDevirtualization = true; - comp->impDevirtualizeCall(call, &method, &methodFlags, &context, nullptr, isLateDevirtualization); + bool explicitTailCall = (call->gtCall.gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; + comp->impDevirtualizeCall(call, &method, &methodFlags, &context, nullptr, isLateDevirtualization, explicitTailCall); } } else if (tree->OperGet() == GT_ASG) diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 47ed202e1bfe..37b506b6afee 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -8590,9 +8590,11 @@ var_types Compiler::impImportCall(OPCODE opcode, assert(obj->gtType == TYP_REF); // See if we can devirtualize. + + bool explicitTailCall = (tailCall & PREFIX_TAILCALL_EXPLICIT) != 0; const bool isLateDevirtualization = false; impDevirtualizeCall(call->AsCall(), &callInfo->hMethod, &callInfo->methodFlags, &callInfo->contextHandle, - &exactContextHnd, isLateDevirtualization); + &exactContextHnd, isLateDevirtualization, explicitTailCall); } if (impIsThis(obj)) @@ -8742,7 +8744,6 @@ var_types Compiler::impImportCall(OPCODE opcode, if (info.compCompHnd->canTailCall(info.compMethodHnd, methHnd, exactCalleeHnd, explicitTailCall)) { - canTailCall = true; if (explicitTailCall) { // In case of explicit tail calls, mark it so that it is not considered @@ -20174,6 +20175,7 @@ bool Compiler::IsMathIntrinsic(GenTree* tree) // contextHandle -- [IN/OUT] context handle for the call. Updated iff call devirtualized. // exactContextHnd -- [OUT] updated context handle iff call devirtualized // isLateDevirtualization -- if devirtualization is happening after importation +// isTailCall -- [IN/OUT] true if we plan on using a tail call // // Notes: // Virtual calls in IL will always "invoke" the base class method. @@ -20207,7 +20209,8 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, unsigned* methodFlags, CORINFO_CONTEXT_HANDLE* contextHandle, CORINFO_CONTEXT_HANDLE* exactContextHandle, - bool isLateDevirtualization) + bool isLateDevirtualization, + bool isTailCall) { assert(call != nullptr); assert(method != nullptr); @@ -20558,6 +20561,12 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, { JITDUMP("Now have direct call to boxed entry point, looking for unboxed entry point\n"); + if (isTailCall) + { + JITDUMP("Call is an explcit tail call, we cannot perform an unbox\n"); + return; + } + // Note for some shared methods the unboxed entry point requires an extra parameter. bool requiresInstMethodTableArg = false; CORINFO_METHOD_HANDLE unboxedEntryMethod = @@ -20640,6 +20649,16 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, call->gtCallMethHnd = unboxedEntryMethod; call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; derivedMethod = unboxedEntryMethod; + + if (call->IsImplicitTailCall()) + { + JITDUMP("Clearing the implicit tail call flag\n"); + + // If set, we clear the implicit tail call flag + // as we just introduced a new address taken local variable + // + call->gtCallMoreFlags &= ~GTF_CALL_M_IMPLICIT_TAILCALL; + } } else { diff --git a/src/jit/indirectcalltransformer.cpp b/src/jit/indirectcalltransformer.cpp index 85d6d227cb9a..53bb33bd7d3e 100644 --- a/src/jit/indirectcalltransformer.cpp +++ b/src/jit/indirectcalltransformer.cpp @@ -676,14 +676,16 @@ class IndirectCallTransformer JITDUMP("Direct call [%06u] in block BB%02u\n", compiler->dspTreeID(call), thenBlock->bbNum); - // Then invoke impDevirtualizeCall do actually + // Then invoke impDevirtualizeCall to actually // transform the call for us. It should succeed.... as we have // now provided an exact typed this. CORINFO_METHOD_HANDLE methodHnd = inlineInfo->methInfo.ftn; unsigned methodFlags = inlineInfo->methAttr; CORINFO_CONTEXT_HANDLE context = inlineInfo->exactContextHnd; const bool isLateDevirtualization = true; - compiler->impDevirtualizeCall(call, &methodHnd, &methodFlags, &context, nullptr, isLateDevirtualization); + bool explicitTailCall = (call->gtCall.gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; + compiler->impDevirtualizeCall(call, &methodHnd, &methodFlags, &context, nullptr, isLateDevirtualization, + explicitTailCall); // Presumably devirt might fail? If so we should try and avoid // making this a guarded devirt candidate instead of ending From eb8762330a703aedfea16189b6f0813e2217638d Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 9 Jan 2019 16:05:45 -0800 Subject: [PATCH 2/5] jit format --- src/jit/flowgraph.cpp | 5 +++-- src/jit/importer.cpp | 6 +++--- src/jit/indirectcalltransformer.cpp | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index feaceb3dcbfb..c6f6bc38f94c 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -22510,8 +22510,9 @@ Compiler::fgWalkResult Compiler::fgLateDevirtualization(GenTree** pTree, fgWalkD unsigned methodFlags = 0; CORINFO_CONTEXT_HANDLE context = nullptr; const bool isLateDevirtualization = true; - bool explicitTailCall = (call->gtCall.gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; - comp->impDevirtualizeCall(call, &method, &methodFlags, &context, nullptr, isLateDevirtualization, explicitTailCall); + bool explicitTailCall = (call->gtCall.gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; + comp->impDevirtualizeCall(call, &method, &methodFlags, &context, nullptr, isLateDevirtualization, + explicitTailCall); } } else if (tree->OperGet() == GT_ASG) diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 37b506b6afee..6396e219e7cd 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -8591,7 +8591,7 @@ var_types Compiler::impImportCall(OPCODE opcode, // See if we can devirtualize. - bool explicitTailCall = (tailCall & PREFIX_TAILCALL_EXPLICIT) != 0; + bool explicitTailCall = (tailCall & PREFIX_TAILCALL_EXPLICIT) != 0; const bool isLateDevirtualization = false; impDevirtualizeCall(call->AsCall(), &callInfo->hMethod, &callInfo->methodFlags, &callInfo->contextHandle, &exactContextHnd, isLateDevirtualization, explicitTailCall); @@ -20175,7 +20175,7 @@ bool Compiler::IsMathIntrinsic(GenTree* tree) // contextHandle -- [IN/OUT] context handle for the call. Updated iff call devirtualized. // exactContextHnd -- [OUT] updated context handle iff call devirtualized // isLateDevirtualization -- if devirtualization is happening after importation -// isTailCall -- [IN/OUT] true if we plan on using a tail call +// isTailCall -- [IN/OUT] true if we plan on using a tail call // // Notes: // Virtual calls in IL will always "invoke" the base class method. @@ -20654,7 +20654,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, { JITDUMP("Clearing the implicit tail call flag\n"); - // If set, we clear the implicit tail call flag + // If set, we clear the implicit tail call flag // as we just introduced a new address taken local variable // call->gtCallMoreFlags &= ~GTF_CALL_M_IMPLICIT_TAILCALL; diff --git a/src/jit/indirectcalltransformer.cpp b/src/jit/indirectcalltransformer.cpp index 53bb33bd7d3e..e0782b5b7350 100644 --- a/src/jit/indirectcalltransformer.cpp +++ b/src/jit/indirectcalltransformer.cpp @@ -683,7 +683,7 @@ class IndirectCallTransformer unsigned methodFlags = inlineInfo->methAttr; CORINFO_CONTEXT_HANDLE context = inlineInfo->exactContextHnd; const bool isLateDevirtualization = true; - bool explicitTailCall = (call->gtCall.gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; + bool explicitTailCall = (call->gtCall.gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; compiler->impDevirtualizeCall(call, &methodHnd, &methodFlags, &context, nullptr, isLateDevirtualization, explicitTailCall); From 381b3703d246a2b83557ec2107d63a0a3ef2cbcd Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 9 Jan 2019 16:37:44 -0800 Subject: [PATCH 3/5] Added ifdef for FEATURE_TAILCALL_OPT --- src/jit/importer.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 6396e219e7cd..be735a200ba6 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -20650,6 +20650,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; derivedMethod = unboxedEntryMethod; +#if FEATURE_TAILCALL_OPT if (call->IsImplicitTailCall()) { JITDUMP("Clearing the implicit tail call flag\n"); @@ -20659,6 +20660,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // call->gtCallMoreFlags &= ~GTF_CALL_M_IMPLICIT_TAILCALL; } +#endif // FEATURE_TAILCALL_OPT } else { From b9fafc63c6d2a90f0444d809269380fd593c03a3 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 10 Jan 2019 11:22:12 -0800 Subject: [PATCH 4/5] New test for DevDiv_754566 --- .../JitBlue/DevDiv_754566/DevDiv_754566.il | 106 ++++++++++++++++++ .../DevDiv_754566/DevDiv_754566.ilprog | 35 ++++++ .../Regression/JitBlue/DevDiv_754566/test.cs | 31 +++++ 3 files changed, 172 insertions(+) create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.il create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.ilprog create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_754566/test.cs diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.il b/tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.il new file mode 100644 index 000000000000..21290d24daf7 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.il @@ -0,0 +1,106 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + + +// Metadata version: v4.0.30319 +.assembly extern mscorlib +{ + .publickeytoken = (B7 7A 5C 56 19 34 E0 89 ) // .z\V.4.. + .ver 4:0:0:0 +} +.assembly test +{ + .custom instance void [mscorlib]System.Runtime.CompilerServices.CompilationRelaxationsAttribute::.ctor(int32) = ( 01 00 08 00 00 00 00 00 ) + .custom instance void [mscorlib]System.Runtime.CompilerServices.RuntimeCompatibilityAttribute::.ctor() = ( 01 00 01 00 54 02 16 57 72 61 70 4E 6F 6E 45 78 // ....T..WrapNonEx + 63 65 70 74 69 6F 6E 54 68 72 6F 77 73 01 ) // ceptionThrows. + .hash algorithm 0x00008004 + .ver 0:0:0:0 +} +.module test.exe +// MVID: {A80A87C4-1DDB-4F93-AB31-444266FDFA55} +.imagebase 0x00400000 +.file alignment 0x00000200 +.stackreserve 0x00100000 +.subsystem 0x0003 // WINDOWS_CUI +.corflags 0x00000001 // ILONLY +// Image base: 0x0000024A58020000 + + +// =============== CLASS MEMBERS DECLARATION =================== + +.class private auto ansi beforefieldinit Program + extends [mscorlib]System.Object +{ + .method public hidebysig instance string + Test(int32 val) cil managed noinlining + { + // This testcase ensures that we don't perform devirtualization + // via an unboxing optimization, for the callvirt below. + + // Code size 12 (0xc) + .maxstack 8 + IL_0000: ldarg.1 + IL_0001: box [mscorlib]System.Int32 + tail. + IL_0006: callvirt instance string [mscorlib]System.Object::ToString() + IL_000b: ret + } // end of method Program::Test + + .method private hidebysig static int32 + Main(string[] args) cil managed + { + .entrypoint + // Code size 73 (0x49) + .maxstack 2 + .locals init (int32 V_0, + class Program V_1, + string V_2) + IL_0000: ldc.i4.m1 + IL_0001: stloc.0 + IL_0002: newobj instance void Program::.ctor() + IL_0007: stloc.1 + IL_0008: ldloc.1 + IL_0009: ldc.i4.s 42 + IL_000b: callvirt instance string Program::Test(int32) + IL_0010: stloc.2 + IL_0011: ldloc.2 + IL_0012: ldstr "42" + IL_0017: call bool [mscorlib]System.String::op_Equality(string, + string) + IL_001c: brfalse.s IL_002d + + IL_001e: ldstr "=== PASSED ===" + IL_0023: call void [mscorlib]System.Console::WriteLine(string) + IL_0028: ldc.i4.s 100 + IL_002a: stloc.0 + IL_002b: br.s IL_0047 + + IL_002d: ldstr "result shoudl be 42, is= " + IL_0032: ldloc.2 + IL_0033: call string [mscorlib]System.String::Concat(string, + string) + IL_0038: call void [mscorlib]System.Console::WriteLine(string) + IL_003d: ldstr "+++ FAILED +++" + IL_0042: call void [mscorlib]System.Console::WriteLine(string) + IL_0047: ldloc.0 + IL_0048: ret + } // end of method Program::Main + + .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 [mscorlib]System.Object::.ctor() + IL_0006: ret + } // end of method Program::.ctor + +} // end of class Program + + +// ============================================================= + +// *********** DISASSEMBLY COMPLETE *********************** +// WARNING: Created Win32 resource file test2.res diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.ilprog b/tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.ilprog new file mode 100644 index 000000000000..9199c02f2d27 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.ilprog @@ -0,0 +1,35 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + 1 + + + + + + PdbOnly + True + + + + False + + + + + + + + + + + \ No newline at end of file diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_754566/test.cs b/tests/src/JIT/Regression/JitBlue/DevDiv_754566/test.cs new file mode 100644 index 000000000000..a07bb7bd3c3f --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/DevDiv_754566/test.cs @@ -0,0 +1,31 @@ +using System; +using System.Runtime.CompilerServices; + +class Program +{ + [MethodImpl(MethodImplOptions.NoInlining)] + public String Test(int val) + { + return ((Object) val).ToString(); + } + + static int Main(string[] args) + { + int exitStatus = -1; + + Program p = new Program(); + + String result = p.Test(42); + if (result == "42") + { + Console.WriteLine("=== PASSED ==="); + exitStatus = 100; + } + else + { + Console.WriteLine("result shoudl be 42, is= " + result); + Console.WriteLine("+++ FAILED +++"); + } + return exitStatus; + } +} From b4cbfba170ccdf277a5080bf24460fde4c41bbf3 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 10 Jan 2019 16:02:10 -0800 Subject: [PATCH 5/5] Code Review feedback Change test priority to 0 --- src/jit/compiler.h | 2 +- src/jit/importer.cpp | 10 +++++----- .../JitBlue/DevDiv_754566/DevDiv_754566.ilprog | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/jit/compiler.h b/src/jit/compiler.h index 5f42bb5a9ce4..7f5133284640 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -3330,7 +3330,7 @@ class Compiler CORINFO_CONTEXT_HANDLE* contextHandle, CORINFO_CONTEXT_HANDLE* exactContextHandle, bool isLateDevirtualization, - bool isTailCall); + bool isExplicitTailCall); //========================================================================= // PROTECTED diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index be735a200ba6..27ea06abafd4 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -5276,7 +5276,7 @@ BOOL Compiler::verIsSDArray(typeInfo ti) typeInfo Compiler::verGetArrayElemType(typeInfo arrayObjectType) { - assert(!arrayObjectType.IsNullObjRef()); // you need to check for null explictly since that is a success case + assert(!arrayObjectType.IsNullObjRef()); // you need to check for null explicitly since that is a success case if (!verIsSDArray(arrayObjectType)) { @@ -20175,7 +20175,7 @@ bool Compiler::IsMathIntrinsic(GenTree* tree) // contextHandle -- [IN/OUT] context handle for the call. Updated iff call devirtualized. // exactContextHnd -- [OUT] updated context handle iff call devirtualized // isLateDevirtualization -- if devirtualization is happening after importation -// isTailCall -- [IN/OUT] true if we plan on using a tail call +// isExplicitTailCalll -- [IN] true if we plan on using an explicit tail call // // Notes: // Virtual calls in IL will always "invoke" the base class method. @@ -20210,7 +20210,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, CORINFO_CONTEXT_HANDLE* contextHandle, CORINFO_CONTEXT_HANDLE* exactContextHandle, bool isLateDevirtualization, - bool isTailCall) + bool isExplicitTailCall) { assert(call != nullptr); assert(method != nullptr); @@ -20561,9 +20561,9 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, { JITDUMP("Now have direct call to boxed entry point, looking for unboxed entry point\n"); - if (isTailCall) + if (isExplicitTailCall) { - JITDUMP("Call is an explcit tail call, we cannot perform an unbox\n"); + JITDUMP("Call is an explicit tail call, we cannot perform an unbox\n"); return; } diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.ilprog b/tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.ilprog index 9199c02f2d27..d30b4aeb0cae 100644 --- a/tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.ilprog +++ b/tests/src/JIT/Regression/JitBlue/DevDiv_754566/DevDiv_754566.ilprog @@ -10,7 +10,7 @@ Exe {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} ..\..\ - 1 + 0