From dcf22411978c6736d4eb757cce894eaf6dab54fa Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 4 Dec 2023 07:24:43 -0800 Subject: [PATCH] JIT: fix issue with out of order importer spilling around some calls (#95539) If we have a call that returns a struct that is immediately assigned to some local, the call is a GDV inline candidate, and the call is invoked with a copy of the same local on the evaluation stack, the spill of that local into a temp will appear in the IR stream between the call and the ret expr, instead of before the call. As part of our IR resolution the store to the local gets "sunk" into the call as the hidden return buffer, so the importer ordering is manifestly incorrect: ``` call &retbuf, ... tmp = retbuf ...ret-expr ...tmp ``` For normal inline candidates this mis-ordering gets fixed up either by swapping the call back into the ret expr's position, or for successful inlines by swapping the return value store into the ret expr's position. The JIT has behaved this way for a very long time, and the transient mis-ordering has not lead to any noticble problem. For GDV calls the return value stores are done earlier, just after the call, and so the spill picks up the wrong value. GDV calls normally only happen with PGO data. This persistent mis-ordering has been the behavior since at least 6.0, but now that PGO is enabled by default a much wider set of programs are at risk of running into it. The fix here is to reorder the IR in the importer at the point the store to the local is appended to the IR stream, as we are processing spills for the local. If the source of the store is a GT_RET_EXPR we keep track of these spills, find the associated GT_CALL statement, and move the spills before the call. There was a similar fix made for boxes in #60335, where once again the splitting of the inline candidate call and the subsequent modification of the call to write directly to the return buffer local led to similar problems with GDV calls. Fixes #95394. --- src/coreclr/jit/importer.cpp | 39 ++++++++++++ .../JitBlue/Runtime_95349/Runtime_95349.cs | 61 +++++++++++++++++++ .../Runtime_95349/Runtime_95349.csproj | 9 +++ 3 files changed, 109 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_95349/Runtime_95349.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_95349/Runtime_95349.csproj diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 722afadcd9a21..a6ddc2a8cb6e9 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -453,6 +453,11 @@ void Compiler::impAppendStmt(Statement* stmt, unsigned chkLevel, bool checkConsu } } + // In the case of GT_RET_EXPR any subsequent spills will appear in the wrong place -- after + // the call. We need to move them to before the call + // + Statement* lastStmt = impLastStmt; + if ((dstVarDsc != nullptr) && !dstVarDsc->IsAddressExposed() && !dstVarDsc->lvHasLdAddrOp) { impSpillLclRefs(lvaGetLclNum(dstVarDsc), chkLevel); @@ -480,6 +485,40 @@ void Compiler::impAppendStmt(Statement* stmt, unsigned chkLevel, bool checkConsu { impSpillSpecialSideEff(); } + + if ((lastStmt != impLastStmt) && expr->OperIs(GT_RET_EXPR)) + { + GenTree* const call = expr->AsRetExpr()->gtInlineCandidate; + JITDUMP("\nimpAppendStmt: after sinking a local struct store into inline candidate [%06u], we need to " + "reorder subsequent spills.\n", + dspTreeID(call)); + + // Move all newly appended statements to just before the call's statement. + // First, find the statement containing the call. + // + Statement* insertBeforeStmt = lastStmt; + + while (insertBeforeStmt->GetRootNode() != call) + { + assert(insertBeforeStmt != impStmtList); + insertBeforeStmt = insertBeforeStmt->GetPrevStmt(); + } + + Statement* movingStmt = lastStmt->GetNextStmt(); + + JITDUMP("Moving " FMT_STMT " through " FMT_STMT " before " FMT_STMT "\n", movingStmt->GetID(), + impLastStmt->GetID(), insertBeforeStmt->GetID()); + + // We move these backwards, so must keep moving the insert + // point to keep them in order. + // + while (impLastStmt != lastStmt) + { + Statement* movingStmt = impExtractLastStmt(); + impInsertStmtBefore(movingStmt, insertBeforeStmt); + insertBeforeStmt = movingStmt; + } + } } impAppendStmtCheck(stmt, chkLevel); diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_95349/Runtime_95349.cs b/src/tests/JIT/Regression/JitBlue/Runtime_95349/Runtime_95349.cs new file mode 100644 index 0000000000000..95cb943a4f6d0 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_95349/Runtime_95349.cs @@ -0,0 +1,61 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Issues with stack spill ordering around some GDVs +// Compile with None + +using System; +using System.Runtime.CompilerServices; +using System.Threading; +using Xunit; + +class P +{ + virtual public (double x, double y) XY() => (0, 0); +} + +class P1 : P +{ + override public (double x, double y) XY() => (1, 2); +} + +public class Runtime_95349 +{ + [MethodImpl(MethodImplOptions.NoInlining)] + static int Problem(P p, int n, (double x, double y) tuple) + { + int wn = 0; + for (int i = 0; i < n; i++) + { + (double x, double y) tupleTmp = tuple; + tuple = p.XY(); + (_, double f) = tupleTmp; + wn = Wn(wn, f, tuple.y); + } + + return wn; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Wn(int wn, double f, double y) + { + wn += (f == -1) ? 1 : 0; + return wn; + } + + [Fact] + public static void Test() + { + P p = new P1(); + int n = 100_000; + for (int i = 0; i < 100; i++) + { + _ = Problem(p, n, (-1, -1)); + Thread.Sleep(30); + } + + int r = Problem(p, n, (-1, -1)); + Console.WriteLine($"r = {r} (expected 1)"); + Assert.Equal(1, r); + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_95349/Runtime_95349.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_95349/Runtime_95349.csproj new file mode 100644 index 0000000000000..1bb887ea34b0f --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_95349/Runtime_95349.csproj @@ -0,0 +1,9 @@ + + + True + None + + + + + \ No newline at end of file