JIT: fix issue with out of order importer spilling around some calls (#95587)

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.

Co-authored-by: Andy Ayers <[email protected]>

Author: github-actions[bot]

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);

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 <DebugType>None</DebugType>
+
+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);
+    }
+}

src/tests/JIT/Regression/JitBlue/Runtime_95349/Runtime_95349.csproj

@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <Optimize>True</Optimize>
+    <DebugType>None</DebugType>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>