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

diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp
index aa19cc1..b21fc12 100644 (file)
--- 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 (file)
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 (file)
index 0000000..6646c68
--- /dev/null
+++ b/src/coreclr/tests/src/jit/opt/Devirtualization/GitHub_9945.csproj
@@ -0,0 +1,45 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <AppDesignerFolder>Properties</AppDesignerFolder>
+    <FileAlignment>512</FileAlignment>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT  .0\UITestExtensionPackages</ReferencePath>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+    <NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
+  </PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
+  </PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="GitHub_9945.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <PropertyGroup>
+    <ProjectJson>$(JitPackagesConfigFileDirectory)minimal\project.json</ProjectJson>
+    <ProjectLockJson>$(JitPackagesConfigFileDirectory)minimal\project.lock.json</ProjectLockJson>
+  </PropertyGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
+  </PropertyGroup> 
+</Project>