From 4f831dfde67e6e13703a92a4a2cfa530f0a04f11 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 3 Mar 2017 20:01:52 -0800 Subject: [PATCH] JIT: change codgen for dup to not lose type information The jit has traditionally optimized most cases of a zz; dup; stloc.x sequence into zz; stloc.x; ldloc.x with a comment noting that this makes the resulting compiler IR more amenable to CSE. Here zz is any IL operation that pushes a value on the stack. However if zz's result is a ref type, the dup'ed value produced by zz may be a subtype of the type of the local x, so this transformation can lose type information. In the past the loss of type information hasn't mattered, but now if zz subsequently is used as the this object in a virtual call, the jit may attempt devirtualization and ask an ill-posed question to the VM, claiming the type of the this object is an impossible supertype. This changes modifies the transformation by introducing a new local temp y that has the type of zz. The result is now zz; stloc.y; ldloc.y; ldloc.y; stloc.x with the net result that a ldloc.y is left on the stack with the proper type. As an optimization, if zz is ldloc.z then the expansion is simply zz (ldloc.z); ldloc.z; stloc.y and if zz is a simple "cheap" constant (say ldnull) then the constant expression is duplicated: zz (ldnull); ldnull; stloc.y This resolves the jit side of dotnet/coreclr#9945. Further work is needed on the VM side since in unverifiable IL there may be other cases where the jit can present an impossible type. Diffs from this were minor and in many cases small improvements. Commit migrated from https://github.com/dotnet/coreclr/commit/743e5e1660b308cba660565419a972146a6b4191 --- src/coreclr/src/jit/importer.cpp | 88 +++------------------- .../src/jit/opt/Devirtualization/GitHub_9945.cs | 68 +++++++++++++++++ .../jit/opt/Devirtualization/GitHub_9945.csproj | 45 +++++++++++ 3 files changed, 123 insertions(+), 78 deletions(-) create mode 100644 src/coreclr/tests/src/jit/opt/Devirtualization/GitHub_9945.cs create mode 100644 src/coreclr/tests/src/jit/opt/Devirtualization/GitHub_9945.csproj diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index aa19cc1..b21fc12 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -9508,7 +9508,6 @@ void Compiler::impImportBlockCode(BasicBlock* block) int prefixFlags = 0; bool explicitTailCall, constraintCall, readonlyCall; - bool insertLdloc = false; // set by CEE_DUP and cleared by following store typeInfo tiRetVal; unsigned numArgs = info.compArgsCount; @@ -10077,27 +10076,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) if (op1->gtOper == GT_LCL_VAR && lclNum == op1->gtLclVarCommon.gtLclNum) { - if (insertLdloc) - { - // This is a sequence of (ldloc, dup, stloc). Can simplify - // to (ldloc, stloc). Goto LDVAR to reconstruct the ldloc node. - CLANG_FORMAT_COMMENT_ANCHOR; - -#ifdef DEBUG - if (tiVerificationNeeded) - { - assert( - typeInfo::AreEquivalent(tiRetVal, NormaliseForStack(lvaTable[lclNum].lvVerTypeInfo))); - } -#endif - - op1 = nullptr; - insertLdloc = false; - - impLoadVar(lclNum, opcodeOffs + sz + 1); - break; - } - else if (opts.compDbgCode) + if (opts.compDbgCode) { op1 = gtNewNothingNode(); goto SPILL_APPEND; @@ -10154,26 +10133,6 @@ void Compiler::impImportBlockCode(BasicBlock* block) op1 = gtNewAssignNode(op2, op1); } - /* If insertLdloc is true, then we need to insert a ldloc following the - stloc. This is done when converting a (dup, stloc) sequence into - a (stloc, ldloc) sequence. */ - - if (insertLdloc) - { - // From SPILL_APPEND - impAppendTree(op1, (unsigned)CHECK_SPILL_ALL, impCurStmtOffs); - -#ifdef DEBUG - // From DONE_APPEND - impNoteLastILoffs(); -#endif - op1 = nullptr; - insertLdloc = false; - - impLoadVar(lclNum, opcodeOffs + sz + 1, tiRetVal); - break; - } - goto SPILL_APPEND; case CEE_LDLOCA: @@ -11991,48 +11950,24 @@ void Compiler::impImportBlockCode(BasicBlock* block) impStackTop(0); } - // Convert a (dup, stloc) sequence into a (stloc, ldloc) sequence in the following cases: - // - If this is non-debug code - so that CSE will recognize the two as equal. - // This helps eliminate a redundant bounds check in cases such as: - // ariba[i+3] += some_value; - // - If the top of the stack is a non-leaf that may be expensive to clone. + // If the expression to dup is simple, just clone it. + // Otherwise spill it to a temp, and reload the temp + // twice. + op1 = impPopStack(tiRetVal); - if (codeAddr < codeEndp) + if (!opts.compDbgCode && !op1->IsIntegralConst(0) && !op1->IsFPZero() && !op1->IsLocal()) { - OPCODE nextOpcode = (OPCODE)getU1LittleEndian(codeAddr); - if (impIsAnySTLOC(nextOpcode)) - { - if (!opts.compDbgCode) - { - insertLdloc = true; - break; - } - GenTree* stackTop = impStackTop().val; - if (!stackTop->IsIntegralConst(0) && !stackTop->IsFPZero() && !stackTop->IsLocal()) - { - insertLdloc = true; - break; - } - } + const unsigned tmpNum = lvaGrabTemp(true DEBUGARG("dup spill")); + impAssignTempGen(tmpNum, op1, tiRetVal.GetClassHandle(), (unsigned)CHECK_SPILL_ALL); + var_types type = genActualType(lvaTable[tmpNum].TypeGet()); + op1 = gtNewLclvNode(tmpNum, type); } - /* Pull the top value from the stack */ - op1 = impPopStack(tiRetVal); - - /* Clone the value */ op1 = impCloneExpr(op1, &op2, tiRetVal.GetClassHandle(), (unsigned)CHECK_SPILL_ALL, nullptr DEBUGARG("DUP instruction")); - /* Either the tree started with no global effects, or impCloneExpr - evaluated the tree to a temp and returned two copies of that - temp. Either way, neither op1 nor op2 should have side effects. - */ assert(!(op1->gtFlags & GTF_GLOB_EFFECT) && !(op2->gtFlags & GTF_GLOB_EFFECT)); - - /* Push the tree/temp back on the stack */ impPushOnStack(op1, tiRetVal); - - /* Push the copy on the stack */ impPushOnStack(op2, tiRetVal); break; @@ -14870,11 +14805,8 @@ void Compiler::impImportBlockCode(BasicBlock* block) prevOpcode = opcode; prefixFlags = 0; - assert(!insertLdloc || opcode == CEE_DUP); } - assert(!insertLdloc); - return; #undef _impResolveToken } diff --git a/src/coreclr/tests/src/jit/opt/Devirtualization/GitHub_9945.cs b/src/coreclr/tests/src/jit/opt/Devirtualization/GitHub_9945.cs new file mode 100644 index 0000000..3574f4f --- /dev/null +++ b/src/coreclr/tests/src/jit/opt/Devirtualization/GitHub_9945.cs @@ -0,0 +1,68 @@ +// 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. + +using System; + +public class Base +{ + public int value; + public void B0() { value += 12; } + public virtual void B1() { value += 33; } +} + +// Ensure that D1 and D2 have enough virtuals that the slot number for +// the virtual M1 is greater than any virtual's slot number in B. + +public class D1 : Base +{ + public virtual void MA() { } + public virtual void MB() { } + public virtual void MC() { } + public virtual void MD() { } + + public virtual void M1() { value += 44; } +} + +public class D2 : Base +{ + public virtual void MA() { } + public virtual void MB() { } + public virtual void MC() { } + public virtual void MD() { } + + public virtual void M1() { value += 55; } +} + +// Aggressive use of 'dup' here by CSC will confuse the jit, and it +// may substitute 'b' for uses of d1 and d2. This is not +// value-incorrect but loses type information. +// +// This loss of type information subsequently triggers an assert in +// devirtualzation because b does not have M1 as virtual method. + +public class Test +{ + public static int Main(string[] args) + { + Base b; + + if (args.Length > 0) + { + D1 d1 = new D1(); + b = d1; + d1.B1(); + d1.M1(); + } + else + { + D2 d2 = new D2(); + b = d2; + d2.B1(); + d2.M1(); + } + + b.B0(); + return b.value; + } +} diff --git a/src/coreclr/tests/src/jit/opt/Devirtualization/GitHub_9945.csproj b/src/coreclr/tests/src/jit/opt/Devirtualization/GitHub_9945.csproj new file mode 100644 index 0000000..6646c68 --- /dev/null +++ b/src/coreclr/tests/src/jit/opt/Devirtualization/GitHub_9945.csproj @@ -0,0 +1,45 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT .0\UITestExtensionPackages + ..\..\ + 7a9bfb7d + + + + + + + + + False + + + + None + True + + + + + + + + + $(JitPackagesConfigFileDirectory)minimal\project.json + $(JitPackagesConfigFileDirectory)minimal\project.lock.json + + + + + -- 2.7.4