Fix evaluation order for block copy

This order was changed five years ago, as a workaround for data flow analysis issues around the block copy. Now that block copies are handled as regular assignments, this is no longer needed - and is, in fact, incorrect as it causes side effects to be handled in the wrong order.

However, for the case where the lhs is an indirection of a local address, it must be evaluated 2nd for SSA renaming to be correct.

In the process, also remove the special-casing for the size operand of `DYN_BLK` under `ASG`.

Fix dotnet/coreclr#19243

Commit migrated from https://github.com/dotnet/coreclr/commit/16e5914706e8865a35da769abe7e6f5da4c910eb

diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp
index d98cf2f..cb7c476 100644 (file)
--- a/src/coreclr/src/jit/flowgraph.cpp
+++ b/src/coreclr/src/jit/flowgraph.cpp
@@ -18494,36 +18494,24 @@ void Compiler::fgSetTreeSeqHelper(GenTree* tree, bool isLIR)
     }
 
     // Special handling for dynamic block ops.
-    if (tree->OperIsDynBlkOp())
+    if (tree->OperIs(GT_DYN_BLK, GT_STORE_DYN_BLK))
     {
-        GenTreeDynBlk* dynBlk;
-        GenTree*       src;
-        GenTree*       asg = tree;
-        if (tree->OperGet() == GT_ASG)
-        {
-            dynBlk = tree->gtGetOp1()->AsDynBlk();
-            src    = tree->gtGetOp2();
-        }
-        else
-        {
-            dynBlk = tree->AsDynBlk();
-            src    = dynBlk->Data();
-            asg    = nullptr;
-        }
-        GenTree* sizeNode = dynBlk->gtDynamicSize;
-        GenTree* dstAddr  = dynBlk->Addr();
+        GenTreeDynBlk* dynBlk    = tree->AsDynBlk();
+        GenTree*       sizeNode  = dynBlk->gtDynamicSize;
+        GenTree*       dstAddr   = dynBlk->Addr();
+        GenTree*       src       = dynBlk->Data();
+        bool           isReverse = ((dynBlk->gtFlags & GTF_REVERSE_OPS) != 0);
         if (dynBlk->gtEvalSizeFirst)
         {
             fgSetTreeSeqHelper(sizeNode, isLIR);
         }
-        if (tree->gtFlags & GTF_REVERSE_OPS)
+        if (isReverse && (src != nullptr))
         {
             fgSetTreeSeqHelper(src, isLIR);
-            fgSetTreeSeqHelper(dstAddr, isLIR);
         }
-        else
+        fgSetTreeSeqHelper(dstAddr, isLIR);
+        if (!isReverse && (src != nullptr))
         {
-            fgSetTreeSeqHelper(dstAddr, isLIR);
             fgSetTreeSeqHelper(src, isLIR);
         }
         if (!dynBlk->gtEvalSizeFirst)
@@ -18531,10 +18519,6 @@ void Compiler::fgSetTreeSeqHelper(GenTree* tree, bool isLIR)
             fgSetTreeSeqHelper(sizeNode, isLIR);
         }
         fgSetTreeSeqFinish(dynBlk, isLIR);
-        if (asg != nullptr)
-        {
-            fgSetTreeSeqFinish(asg, isLIR);
-        }
         return;
     }
 

diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp
index af59dfe..f9844d6 100644 (file)
--- a/src/coreclr/src/jit/gentree.cpp
+++ b/src/coreclr/src/jit/gentree.cpp
@@ -4203,17 +4203,33 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree)
             switch (op1Val->gtOper)
             {
                 case GT_IND:
+                case GT_BLK:
+                case GT_OBJ:
+                case GT_DYN_BLK:
 
-                    // Struct assignments are different from scalar assignments in that semantically
-                    // the address of op1 is evaluated prior to op2.
-                    if (!varTypeIsStruct(op1))
+                    // In an indirection, the destination address is evaluated prior to the source.
+                    // If we have any side effects on the target indirection,
+                    // we have to evaluate op1 first.
+                    // However, if the LHS is a lclVar address, SSA relies on using evaluation order for its
+                    // renaming, and therefore the RHS must be evaluated first.
+                    // If we have an assignment involving a lclVar address, the LHS may be marked as having
+                    // side-effects.
+                    // However the side-effects won't require that we evaluate the LHS address first:
+                    // - The GTF_GLOB_REF might have been conservatively set on a FIELD of a local.
+                    // - The local might be address-exposed, but that side-effect happens at the actual assignment (not
+                    //   when its address is "evaluated") so it doesn't change the side effect to "evaluate" the address
+                    //   after the RHS (note that in this case it won't be renamed by SSA anyway, but the reordering is
+                    //   safe).
+                    //
+                    if (op1Val->AsIndir()->Addr()->IsLocalAddrExpr())
                     {
-                        // If we have any side effects on the GT_IND child node
-                        // we have to evaluate op1 first.
-                        if (op1Val->gtOp.gtOp1->gtFlags & GTF_ALL_EFFECT)
-                        {
-                            break;
-                        }
+                        bReverseInAssignment = true;
+                        tree->gtFlags |= GTF_REVERSE_OPS;
+                        break;
+                    }
+                    if (op1Val->AsIndir()->Addr()->gtFlags & GTF_ALL_EFFECT)
+                    {
+                        break;
                     }
 
                     // In case op2 assigns to a local var that is used in op1Val, we have to evaluate op1Val first.
@@ -4233,9 +4249,6 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree)
 
                 case GT_LCL_VAR:
                 case GT_LCL_FLD:
-                case GT_BLK:
-                case GT_OBJ:
-                case GT_DYN_BLK:
 
                     // We evaluate op2 before op1
                     bReverseInAssignment = true;
@@ -6912,9 +6925,6 @@ void Compiler::gtBlockOpInit(GenTree* result, GenTree* dst, GenTree* srcOrFillVa
     result->gtFlags |= dst->gtFlags & GTF_ALL_EFFECT;
     result->gtFlags |= result->gtOp.gtOp2->gtFlags & GTF_ALL_EFFECT;
 
-    // REVERSE_OPS is necessary because the use must occur before the def
-    result->gtFlags |= GTF_REVERSE_OPS;
-
     result->gtFlags |= (dst->gtFlags & GTF_EXCEPT) | (srcOrFillVal->gtFlags & GTF_EXCEPT);
 
     if (isVolatile)
@@ -8411,15 +8421,6 @@ unsigned GenTree::NumChildren()
             }
         }
 #endif
-        // Special case for assignment of dynamic block.
-        // This is here to duplicate the former case where the size may be evaluated prior to the
-        // source and destination addresses. In order to do this, we treat the size as a child of the
-        // assignment.
-        // TODO-1stClassStructs-Cleanup: Remove all this special casing, and ensure that the diffs are reasonable.
-        if ((OperGet() == GT_ASG) && (gtOp.gtOp1->OperGet() == GT_DYN_BLK) && (gtOp.gtOp1->AsDynBlk()->gtEvalSizeFirst))
-        {
-            return 3;
-        }
         assert(gtOp.gtOp1 != nullptr);
         if (gtOp.gtOp2 == nullptr)
         {
@@ -8454,17 +8455,8 @@ unsigned GenTree::NumChildren()
             case GT_ARR_ELEM:
                 return 1 + AsArrElem()->gtArrRank;
 
-            // This really has two children, but if the size is evaluated first, we treat it as a child of the
-            // parent assignment.
             case GT_DYN_BLK:
-                if (AsDynBlk()->gtEvalSizeFirst)
-                {
-                    return 1;
-                }
-                else
-                {
-                    return 2;
-                }
+                return 2;
 
             case GT_ARR_OFFSET:
             case GT_STORE_DYN_BLK:
@@ -8604,10 +8596,9 @@ GenTree* GenTree::GetChild(unsigned childNum)
                 switch (childNum)
                 {
                     case 0:
-                        return AsDynBlk()->Addr();
+                        return AsDynBlk()->gtEvalSizeFirst ? AsDynBlk()->gtDynamicSize : AsDynBlk()->Addr();
                     case 1:
-                        assert(!AsDynBlk()->gtEvalSizeFirst);
-                        return AsDynBlk()->gtDynamicSize;
+                        return AsDynBlk()->gtEvalSizeFirst ? AsDynBlk()->Addr() : AsDynBlk()->gtDynamicSize;
                     default:
                         unreached();
                 }

diff --git a/src/coreclr/src/jit/rationalize.cpp b/src/coreclr/src/jit/rationalize.cpp
index 89e4167..8a4115c 100644 (file)
--- a/src/coreclr/src/jit/rationalize.cpp
+++ b/src/coreclr/src/jit/rationalize.cpp
@@ -551,7 +551,10 @@ void Rationalizer::RewriteAssignment(LIR::Use& use)
                 (assignment->gtFlags & (GTF_ALL_EFFECT | GTF_BLK_VOLATILE | GTF_BLK_UNALIGNED | GTF_DONT_CSE));
             storeBlk->gtBlk.Data() = value;
 
-            // Replace the assignment node with the store
+            // Remove the block node from its current position and replace the assignment node with it
+            // (now in its store form).
+            BlockRange().Remove(storeBlk);
+            BlockRange().InsertBefore(assignment, storeBlk);
             use.ReplaceWith(comp, storeBlk);
             BlockRange().Remove(assignment);
             DISPTREERANGE(BlockRange(), use.Def());

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243.cs
new file mode 100644 (file)
index 0000000..68cd912
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243.cs
@@ -0,0 +1,46 @@
+// 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.
+//
+// This test exposed a bug with the ordering of evaluation of a cpblk.
+
+struct S0
+{
+    public long F0;
+    public sbyte F4;
+    public S0(long f0): this() { F0 = f0; }
+}
+
+class C0
+{
+    public S0 F5;
+    public C0(S0 f5) { F5 = f5; }
+}
+
+public class GitHub_19243
+{
+    static C0 s_13 = new C0(new S0(0));
+    static S0 s_37;
+
+    public static int checkValue(long value)
+    {
+        if (value != 8614979244451975600L)
+        {
+            System.Console.WriteLine("s_37.F0 was " + value + "; expected 8614979244451975600L");
+            return -1;
+        }
+        return 100;
+    }
+
+    static ref S0 M7()
+    {
+        s_13 = new C0(new S0(8614979244451975600L));
+        return ref s_37;
+    }
+
+    public static int Main()
+    {
+        M7() = s_13.F5;
+        return checkValue(s_37.F0);
+    }
+}

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_r.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_r.csproj
new file mode 100644 (file)
index 0000000..82611b7
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_r.csproj
@@ -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>{ADEEA3D1-B67B-456E-8F2B-6DCCACC2D34C}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+    <CLRTestPriority>1</CLRTestPriority>
+  </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>False</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="GitHub_19243.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_ro.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_ro.csproj
new file mode 100644 (file)
index 0000000..15d9d64
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_ro.csproj
@@ -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>{ADEEA3D1-B67B-456E-8F2B-6DCCACC2D34C}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+    <CLRTestPriority>1</CLRTestPriority>
+  </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_19243.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>