From 5ca638f3b25336ae0aa9b3b3edd6b67c11a2533c Mon Sep 17 00:00:00 2001 From: Mike Danes Date: Thu, 13 Sep 2018 22:24:04 +0300 Subject: [PATCH] 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. --- src/jit/importer.cpp | 77 ++++++++++++------- .../JitBlue/GitHub_19583/GitHub_19583.cs | 86 ++++++++++++++++++++++ .../JitBlue/GitHub_19583/GitHub_19583.csproj | 16 ++++ 3 files changed, 151 insertions(+), 28 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_19583/GitHub_19583.cs create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_19583/GitHub_19583.csproj diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 27ea06a..e02eaec 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 0000000..2fefddb --- /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 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 0000000..1ef9989 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_19583/GitHub_19583.csproj @@ -0,0 +1,16 @@ + + + + + $(MSBuildProjectName) + Exe + None + True + True + + + + + + + -- 2.7.4