Fix importer spilling in the presence of assignment side effects

Atomic ops like GT_CMPXCHG and some HW intrinsic nodes act like assignements so impAppendStmt has to account for them. They can be top level nodes or they can appear in the RHS of a GT_ASG node that perhaps is not considered to have an assignment side effect itself.

3 files changed, 151 insertions, 28 deletions

diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp
index 27ea06abaf..e02eaec0d5 100644
--- a/src/jit/importer.cpp
+++ b/src/jit/importer.cpp
@@ -561,46 +561,64 @@ inline void Compiler::impAppendStmt(GenTree* stmt, unsigned chkLevel)
     assert(stmt->gtOper == GT_STMT);
     noway_assert(impTreeLast != nullptr);
 
-    /* If the statement being appended has any side-effects, check the stack
-       to see if anything needs to be spilled to preserve correct ordering. */
-
-    GenTree* expr  = stmt->gtStmt.gtStmtExpr;
-    unsigned flags = expr->gtFlags & GTF_GLOB_EFFECT;
-
-    // Assignment to (unaliased) locals don't count as a side-effect as
-    // we handle them specially using impSpillLclRefs(). Temp locals should
-    // be fine too.
-
-    if ((expr->gtOper == GT_ASG) && (expr->gtOp.gtOp1->gtOper == GT_LCL_VAR) &&
-        !(expr->gtOp.gtOp1->gtFlags & GTF_GLOB_REF) && !gtHasLocalsWithAddrOp(expr->gtOp.gtOp2))
-    {
-        unsigned op2Flags = expr->gtOp.gtOp2->gtFlags & GTF_GLOB_EFFECT;
-        assert(flags == (op2Flags | GTF_ASG));
-        flags = op2Flags;
-    }
-
     if (chkLevel == (unsigned)CHECK_SPILL_ALL)
     {
         chkLevel = verCurrentState.esStackDepth;
     }
 
-    if (chkLevel && chkLevel != (unsigned)CHECK_SPILL_NONE)
+    if ((chkLevel != 0) && (chkLevel != (unsigned)CHECK_SPILL_NONE))
     {
         assert(chkLevel <= verCurrentState.esStackDepth);
 
-        if (flags)
+        /* If the statement being appended has any side-effects, check the stack
+           to see if anything needs to be spilled to preserve correct ordering. */
+
+        GenTree* expr  = stmt->gtStmt.gtStmtExpr;
+        unsigned flags = expr->gtFlags & GTF_GLOB_EFFECT;
+
+        // Assignment to (unaliased) locals don't count as a side-effect as
+        // we handle them specially using impSpillLclRefs(). Temp locals should
+        // be fine too.
+
+        if ((expr->gtOper == GT_ASG) && (expr->gtOp.gtOp1->gtOper == GT_LCL_VAR) &&
+            ((expr->gtOp.gtOp1->gtFlags & GTF_GLOB_REF) == 0) && !gtHasLocalsWithAddrOp(expr->gtOp.gtOp2))
+        {
+            unsigned op2Flags = expr->gtOp.gtOp2->gtFlags & GTF_GLOB_EFFECT;
+            assert(flags == (op2Flags | GTF_ASG));
+            flags = op2Flags;
+        }
+
+        if (flags != 0)
         {
-            // If there is a call, we have to spill global refs
-            bool spillGlobEffects = (flags & GTF_CALL) ? true : false;
+            bool spillGlobEffects = false;
 
-            if (expr->gtOper == GT_ASG)
+            if ((flags & GTF_CALL) != 0)
+            {
+                // If there is a call, we have to spill global refs
+                spillGlobEffects = true;
+            }
+            else if (!expr->OperIs(GT_ASG))
+            {
+                if ((flags & GTF_ASG) != 0)
+                {
+                    // The expression is not an assignment node but it has an assignment side effect, it
+                    // must be an atomic op, HW intrinsic or some other kind of node that stores to memory.
+                    // Since we don't know what it assigns to, we need to spill global refs.
+                    spillGlobEffects = true;
+                }
+            }
+            else
             {
                 GenTree* lhs = expr->gtGetOp1();
-                // If we are assigning to a global ref, we have to spill global refs on stack.
-                // TODO-1stClassStructs: Previously, spillGlobEffects was set to true for
-                // GT_INITBLK and GT_COPYBLK, but this is overly conservative, and should be
-                // revisited. (Note that it was NOT set to true for GT_COPYOBJ.)
-                if (!expr->OperIsBlkOp())
+                GenTree* rhs = expr->gtGetOp2();
+
+                if (((rhs->gtFlags | lhs->gtFlags) & GTF_ASG) != 0)
+                {
+                    // Either side of the assignment node has an assignment side effect.
+                    // Since we don't know what it assigns to, we need to spill global refs.
+                    spillGlobEffects = true;
+                }
+                else if (!expr->OperIsBlkOp())
                 {
                     // If we are assigning to a global ref, we have to spill global refs on stack
                     if ((lhs->gtFlags & GTF_GLOB_REF) != 0)
@@ -612,6 +630,9 @@ inline void Compiler::impAppendStmt(GenTree* stmt, unsigned chkLevel)
                          ((lhs->OperGet() == GT_LCL_VAR) &&
                           (lvaTable[lhs->AsLclVarCommon()->gtLclNum].lvStructGcCount == 0)))
                 {
+                    // TODO-1stClassStructs: Previously, spillGlobEffects was set to true for
+                    // GT_INITBLK and GT_COPYBLK, but this is overly conservative, and should be
+                    // revisited. (Note that it was NOT set to true for GT_COPYOBJ.)
                     spillGlobEffects = true;
                 }
             }
diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_19583/GitHub_19583.cs b/tests/src/JIT/Regression/JitBlue/GitHub_19583/GitHub_19583.cs
new file mode 100644
index 0000000000..2fefddb0a3
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_19583/GitHub_19583.cs
@@ -0,0 +1,86 @@
+// 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;
+using System.Runtime.CompilerServices;
+using System.Runtime.Intrinsics;
+using System.Runtime.Intrinsics.X86;
+using System.Threading;
+
+// Check for proper stack spilling in the presence of assignment-like nodes.
+
+public class Program
+{
+    static int Main()
+    {
+        return 100 +
+            (Test1.Run() == 0 ? 0 : 1) +
+            (Test2.Run() == 1 ? 0 : 2) +
+            (Test3.Run() == 0 ? 0 : 4) +
+            (Test4.Run() == 0 ? 0 : 8);
+    }
+
+    class Test1
+    {
+        static long s_1;
+        static int s_3;
+        public static int Run()
+        {
+            int vr16 = s_3;
+            int vr19 = Interlocked.Exchange(ref s_3, 1);
+            return vr16;
+        }
+    }
+
+    class Test2
+    {
+        static int s_32;
+        static int s_46 = 1;
+        public static int Run()
+        {
+            s_32 = 0;
+            M5();
+            return s_46;
+        }
+
+        static void M5()
+        {
+            s_46 *= (Interlocked.Exchange(ref s_46, 0) | s_32--);
+        }
+    }
+
+    class Test3
+    {
+        static int s_3;
+        static int s_11;
+        public static int Run()
+        {
+            return M9(s_3, Interlocked.Exchange(ref s_3, 1), s_11++);
+        }
+
+        static int M9(int arg2, int arg3, int arg4)
+        {
+            return arg2;
+        }
+    }
+
+    class Test4
+    {
+        struct vec
+        {
+            public int x, y, z, w;
+        }
+
+        public static unsafe int Run()
+        {
+            if (!Sse2.IsSupported) return 0;
+
+            vec v = new vec();
+            Vector128<int> o = Sse2.SetAllVector128(1);
+            int vr16 = v.y;
+            Sse2.Store(&v.x, o);
+            return vr16;
+        }
+    }
+}
diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_19583/GitHub_19583.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_19583/GitHub_19583.csproj
new file mode 100644
index 0000000000..1ef998940b
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_19583/GitHub_19583.csproj
@@ -0,0 +1,16 @@
+<?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>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <OutputType>Exe</OutputType>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>