Fix a number of reordering issues in shift decomposition.

Shift decompostition contained a fair number of IR rewrites that were
unintentionally reordering nodes. This change fixes these instances s.t.
they no longer reorder code.

diff --git a/src/jit/decomposelongs.cpp b/src/jit/decomposelongs.cpp
index a64a382..2e70282 100644 (file)
--- a/src/jit/decomposelongs.cpp
+++ b/src/jit/decomposelongs.cpp
@@ -1013,15 +1013,25 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
 {
     assert(use.IsInitialized());
 
-    GenTree* tree      = use.Def();
-    GenTree* gtLong    = tree->gtGetOp1();
+    GenTree* shift     = use.Def();
+    GenTree* gtLong    = shift->gtGetOp1();
     GenTree* loOp1     = gtLong->gtGetOp1();
     GenTree* hiOp1     = gtLong->gtGetOp2();
-    GenTree* shiftByOp = tree->gtGetOp2();
+    GenTree* shiftByOp = shift->gtGetOp2();
 
-    genTreeOps oper        = tree->OperGet();
+    genTreeOps oper        = shift->OperGet();
     genTreeOps shiftByOper = shiftByOp->OperGet();
 
+    // tLo = ...
+    // ...
+    // tHi = ...
+    // ...
+    // tLong = long tLo, tHi
+    // ...
+    // tShiftAmount = ...
+    // ...
+    // tShift = shift tLong, tShiftAmount
+
     assert((oper == GT_LSH) || (oper == GT_RSH) || (oper == GT_RSZ));
 
     // If we are shifting by a constant int, we do not want to use a helper, instead, we decompose.
@@ -1032,9 +1042,9 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
 
         if (count == 0)
         {
-            GenTree* next = tree->gtNext;
-            // Remove tree and don't do anything else.
-            Range().Remove(tree);
+            GenTree* next = shift->gtNext;
+            // Remove shift and don't do anything else.
+            Range().Remove(shift);
             use.ReplaceWith(m_compiler, gtLong);
             return next;
         }
@@ -1048,15 +1058,27 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
         {
             case GT_LSH:
             {
-                Range().Remove(hiOp1);
                 if (count < 32)
                 {
-                    // Hi is a GT_LSH_HI, lo is a GT_LSH. Will produce:
-                    // reg1 = lo
-                    // shl lo, shift
-                    // shld hi, reg1, shift
+                    // For shifts of < 32 bits, we transform the code to:
+                    //
+                    //     tLo = ...
+                    //           st.lclVar vLo, tLo
+                    //     ...
+                    //     tHi = ...
+                    //     ...
+                    //     tShiftLo = lsh vLo, tShiftAmountLo
+                    //     tShitHiLong = long vLo, tHi
+                    //     tShiftHi = lsh_hi tShiftHiLong, tShiftAmountHi
+                    //
+                    // This will produce:
+                    //
+                    //     reg1 = lo
+                    //     shl lo, shift
+                    //     shld hi, reg1, shift
 
                     Range().Remove(gtLong);
+
                     loOp1                = RepresentOpAsLocalVar(loOp1, gtLong, &gtLong->gtOp.gtOp1);
                     unsigned loOp1LclNum = loOp1->AsLclVarCommon()->gtLclNum;
                     Range().Remove(loOp1);
@@ -1074,16 +1096,25 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
 
                     m_compiler->lvaIncRefCnts(loCopy);
 
-                    Range().InsertBefore(tree, loCopy, hiOp1, hiOp);
-                    Range().InsertBefore(tree, shiftByHi, hiResult);
-                    Range().InsertBefore(tree, loOp1, shiftByLo, loResult);
+                    Range().InsertBefore(shift, loOp1, shiftByLo, loResult);
+                    Range().InsertBefore(shift, loCopy, hiOp, shiftByHi, hiResult);
 
-                    insertAfter = loResult;
+                    insertAfter = hiResult;
                 }
                 else
                 {
                     assert(count >= 32);
 
+                    // Since we're left shifting at least 32 bits, we can remove the hi part of the shifted value iff
+                    // it has no side effects.
+                    //
+                    // TODO-CQ: we could go perform this removal transitively (i.e. iteratively remove everything that
+                    // feeds the hi operand while there are no side effects)
+                    if ((hiOp1->gtFlags & GTF_ALL_EFFECT) == 0)
+                    {
+                        Range().Remove(hiOp1);
+                    }
+
                     if (count < 64)
                     {
                         if (count == 32)
@@ -1102,7 +1133,6 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                         else
                         {
                             Range().Remove(gtLong);
-                            Range().Remove(loOp1);
                             assert(count > 32 && count < 64);
 
                             // Move loOp1 into hiResult, do a GT_LSH with count - 32.
@@ -1110,23 +1140,33 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                             // temp
                             GenTree* shiftBy = m_compiler->gtNewIconNode(count - 32, TYP_INT);
                             hiResult         = m_compiler->gtNewOperNode(oper, TYP_INT, loOp1, shiftBy);
-                            Range().InsertBefore(tree, loOp1, shiftBy, hiResult);
+                            Range().InsertBefore(shift, shiftBy, hiResult);
                         }
                     }
                     else
                     {
-                        Range().Remove(gtLong);
-                        Range().Remove(loOp1);
                         assert(count >= 64);
 
+                        Range().Remove(gtLong);
+
+                        // Since we're left shifting at least 64 bits, we can remove the lo part of the shifted value iff
+                        // it has no side effects.
+                        //
+                        // TODO-CQ: we could go perform this removal transitively (i.e. iteratively remove everything that
+                        // feeds the lo operand while there are no side effects)
+                        if ((loOp1->gtFlags & GTF_ALL_EFFECT) == 0)
+                        {
+                            Range().Remove(loOp1);
+                        }
+
                         // Zero out hi (shift of >= 64 bits moves all the bits out of the two registers)
                         hiResult = m_compiler->gtNewZeroConNode(TYP_INT);
-                        Range().InsertBefore(tree, hiResult);
+                        Range().InsertBefore(shift, hiResult);
                     }
 
                     // Zero out loResult (shift of >= 32 bits shifts all lo bits to hiResult)
                     loResult = m_compiler->gtNewZeroConNode(TYP_INT);
-                    Range().InsertBefore(tree, loResult);
+                    Range().InsertBefore(shift, loResult);
 
                     insertAfter = loResult;
                 }
@@ -1159,14 +1199,22 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                     GenTree* loOp = new (m_compiler, GT_LONG) GenTreeOp(GT_LONG, TYP_LONG, loOp1, hiCopy);
                     loResult      = m_compiler->gtNewOperNode(GT_RSH_LO, TYP_INT, loOp, shiftByLo);
 
-                    Range().InsertBefore(tree, hiCopy, loOp);
-                    Range().InsertBefore(tree, shiftByLo, loResult);
-                    Range().InsertBefore(tree, shiftByHi, hiResult);
+                    Range().InsertBefore(shift, hiCopy, loOp);
+                    Range().InsertBefore(shift, shiftByLo, loResult);
+                    Range().InsertBefore(shift, shiftByHi, hiResult);
                 }
                 else
                 {
-                    Range().Remove(loOp1);
-                    Range().Remove(hiOp1);
+                    // Since we're right shifting at least 32 bits, we can remove the lo part of the shifted value iff
+                    // it has no side effects.
+                    //
+                    // TODO-CQ: we could go perform this removal transitively (i.e. iteratively remove everything that
+                    // feeds the lo operand while there are no side effects)
+                    if ((loOp1->gtFlags & GTF_ALL_EFFECT) == 0)
+                    {
+                        Range().Remove(loOp1);
+                    }
+
                     assert(count >= 32);
                     if (count < 64)
                     {
@@ -1174,7 +1222,6 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                         {
                             // Move hiOp1 into loResult.
                             loResult = hiOp1;
-                            Range().InsertBefore(tree, loResult);
                         }
                         else
                         {
@@ -1183,21 +1230,31 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                             // Move hiOp1 into loResult, do a GT_RSZ with count - 32.
                             GenTree* shiftBy = m_compiler->gtNewIconNode(count - 32, TYP_INT);
                             loResult         = m_compiler->gtNewOperNode(oper, TYP_INT, hiOp1, shiftBy);
-                            Range().InsertBefore(tree, hiOp1, shiftBy, loResult);
+                            Range().InsertBefore(shift, shiftBy, loResult);
                         }
                     }
                     else
                     {
                         assert(count >= 64);
 
+                        // Since we're right shifting at least 64 bits, we can remove the hi part of the shifted value iff
+                        // it has no side effects.
+                        //
+                        // TODO-CQ: we could go perform this removal transitively (i.e. iteratively remove everything that
+                        // feeds the hi operand while there are no side effects)
+                        if ((hiOp1->gtFlags & GTF_ALL_EFFECT) == 0)
+                        {
+                            Range().Remove(hiOp1);
+                        }
+
                         // Zero out lo
                         loResult = m_compiler->gtNewZeroConNode(TYP_INT);
-                        Range().InsertBefore(tree, loResult);
+                        Range().InsertBefore(shift, loResult);
                     }
 
                     // Zero out hi
                     hiResult = m_compiler->gtNewZeroConNode(TYP_INT);
-                    Range().InsertBefore(tree, hiResult);
+                    Range().InsertBefore(shift, hiResult);
                 }
 
                 insertAfter = hiResult;
@@ -1206,7 +1263,6 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
             case GT_RSH:
             {
                 Range().Remove(gtLong);
-                Range().Remove(loOp1);
 
                 hiOp1                = RepresentOpAsLocalVar(hiOp1, gtLong, &gtLong->gtOp.gtOp2);
                 unsigned hiOp1LclNum = hiOp1->AsLclVarCommon()->gtLclNum;
@@ -1231,20 +1287,31 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                     GenTree* loOp = new (m_compiler, GT_LONG) GenTreeOp(GT_LONG, TYP_LONG, loOp1, hiCopy);
                     loResult      = m_compiler->gtNewOperNode(GT_RSH_LO, TYP_INT, loOp, shiftByLo);
 
-                    Range().InsertBefore(tree, loOp1, hiCopy, loOp);
-                    Range().InsertBefore(tree, shiftByLo, loResult);
-                    Range().InsertBefore(tree, shiftByHi, hiOp1, hiResult);
+                    Range().InsertBefore(shift, hiCopy, loOp);
+                    Range().InsertBefore(shift, shiftByLo, loResult);
+                    Range().InsertBefore(shift, shiftByHi, hiOp1, hiResult);
                 }
                 else
                 {
                     assert(count >= 32);
+
+                    // Since we're right shifting at least 32 bits, we can remove the lo part of the shifted value iff
+                    // it has no side effects.
+                    //
+                    // TODO-CQ: we could go perform this removal transitively (i.e. iteratively remove everything that
+                    // feeds the lo operand while there are no side effects)
+                    if ((loOp1->gtFlags & GTF_ALL_EFFECT) == 0)
+                    {
+                        Range().Remove(loOp1);
+                    }
+
                     if (count < 64)
                     {
                         if (count == 32)
                         {
                             // Move hiOp1 into loResult.
                             loResult = hiOp1;
-                            Range().InsertBefore(tree, loResult);
+                            Range().InsertBefore(shift, loResult);
                         }
                         else
                         {
@@ -1253,13 +1320,13 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                             // Move hiOp1 into loResult, do a GT_RSH with count - 32.
                             GenTree* shiftBy = m_compiler->gtNewIconNode(count - 32, TYP_INT);
                             loResult         = m_compiler->gtNewOperNode(oper, TYP_INT, hiOp1, shiftBy);
-                            Range().InsertBefore(tree, hiOp1, shiftBy, loResult);
+                            Range().InsertBefore(shift, hiOp1, shiftBy, loResult);
                         }
 
                         // Propagate sign bit in hiResult
                         GenTree* shiftBy = m_compiler->gtNewIconNode(31, TYP_INT);
                         hiResult         = m_compiler->gtNewOperNode(GT_RSH, TYP_INT, hiCopy, shiftBy);
-                        Range().InsertBefore(tree, shiftBy, hiCopy, hiResult);
+                        Range().InsertBefore(shift, shiftBy, hiCopy, hiResult);
 
                         m_compiler->lvaIncRefCnts(hiCopy);
                     }
@@ -1270,12 +1337,12 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                         // Propagate sign bit in loResult
                         GenTree* loShiftBy = m_compiler->gtNewIconNode(31, TYP_INT);
                         loResult           = m_compiler->gtNewOperNode(GT_RSH, TYP_INT, hiCopy, loShiftBy);
-                        Range().InsertBefore(tree, hiCopy, loShiftBy, loResult);
+                        Range().InsertBefore(shift, hiCopy, loShiftBy, loResult);
 
                         // Propagate sign bit in hiResult
                         GenTree* shiftBy = m_compiler->gtNewIconNode(31, TYP_INT);
                         hiResult         = m_compiler->gtNewOperNode(GT_RSH, TYP_INT, hiOp1, shiftBy);
-                        Range().InsertBefore(tree, shiftBy, hiOp1, hiResult);
+                        Range().InsertBefore(shift, shiftBy, hiOp1, hiResult);
 
                         m_compiler->lvaIncRefCnts(hiCopy);
                     }
@@ -1288,15 +1355,16 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                 unreached();
         }
 
-        // Remove tree from Range
-        Range().Remove(tree);
+        // Remove shift from Range
+        Range().Remove(shift);
 
         return FinalizeDecomposition(use, loResult, hiResult, insertAfter);
     }
     else
     {
-        // arguments are single used, but LIR call can work only with local vars.
-        shiftByOp = RepresentOpAsLocalVar(shiftByOp, tree, &tree->gtOp.gtOp2);
+        // Because calls must be created as HIR and lowered to LIR, we need to dump
+        // any LIR temps into lclVars before using them as arguments.
+        shiftByOp = RepresentOpAsLocalVar(shiftByOp, shift, &shift->gtOp.gtOp2);
         loOp1     = RepresentOpAsLocalVar(loOp1, gtLong, &gtLong->gtOp.gtOp1);
         hiOp1     = RepresentOpAsLocalVar(hiOp1, gtLong, &gtLong->gtOp.gtOp2);
 
@@ -1325,16 +1393,16 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
         GenTreeArgList* argList = m_compiler->gtNewArgList(loOp1, hiOp1, shiftByOp);
 
         GenTree* call = m_compiler->gtNewHelperCallNode(helper, TYP_LONG, 0, argList);
-        call->gtFlags |= tree->gtFlags & GTF_ALL_EFFECT;
+        call->gtFlags |= shift->gtFlags & GTF_ALL_EFFECT;
 
         GenTreeCall*    callNode    = call->AsCall();
         ReturnTypeDesc* retTypeDesc = callNode->GetReturnTypeDesc();
         retTypeDesc->InitializeLongReturnType(m_compiler);
 
         call = m_compiler->fgMorphArgs(callNode);
-        Range().InsertAfter(tree, LIR::SeqTree(m_compiler, call));
+        Range().InsertAfter(shift, LIR::SeqTree(m_compiler, call));
 
-        Range().Remove(tree);
+        Range().Remove(shift);
         use.ReplaceWith(m_compiler, call);
         return call;
     }

diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_406158/DevDiv_406158.il b/tests/src/JIT/Regression/JitBlue/DevDiv_406158/DevDiv_406158.il
new file mode 100644 (file)
index 0000000..ad0dc1f
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/DevDiv_406158/DevDiv_406158.il
@@ -0,0 +1,49 @@
+// 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.
+
+.assembly extern mscorlib{}
+.assembly ILGEN_MODULE{}
+.class ILGEN_CLASS
+{
+    .method static unsigned int8 ILGEN_METHOD(int32)
+    {
+        .maxstack  65535
+        .locals init (int16, char)
+        IL_0000: ldc.r8 float64(0x7c9006f1fe68a964)
+        IL_0009: conv.r4
+        IL_000a: conv.ovf.u4
+        IL_000b: nop
+        IL_000c: conv.ovf.i8.un
+        IL_000d: conv.ovf.i8.un
+        IL_000e: ldc.i8 0xa152fc5b9fbc6fbc
+        IL_0017: ldc.i8 0x5a9c2f46c68a4c09
+        IL_0020: cgt.un
+        IL_0022: shr
+        IL_0023: conv.ovf.i8
+        IL_0024: conv.u
+        IL_0025: ret
+    }
+
+    .method static int32 Main()
+    {
+        .entrypoint
+
+        .try
+        {
+            ldc.i4 0
+            call unsigned int8 ILGEN_CLASS::ILGEN_METHOD(int32)
+            pop
+            leave done
+        }
+        catch [mscorlib]System.Exception
+        {
+            pop
+            leave done
+        }
+
+    done:
+        ldc.i4 100
+        ret
+    }
+}

diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_406158/DevDiv_406158.ilproj b/tests/src/JIT/Regression/JitBlue/DevDiv_406158/DevDiv_406158.ilproj
new file mode 100644 (file)
index 0000000..09944f5
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/DevDiv_406158/DevDiv_406158.ilproj
@@ -0,0 +1,35 @@
+<?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>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup>
+  <PropertyGroup>
+    <DebugType>PdbOnly</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup></PropertyGroup>
+  <ItemGroup>
+    <Compile Include="DevDiv_406158.il" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>