[release/9.0-staging] JIT: Read back all replacements before statements with implicit EH control flow

src/coreclr/jit/promotion.cpp

@@ -1885,41 +1885,79 @@ void ReplaceVisitor::InsertPreStatementReadBacks()
     // 3. Creating embedded stores in ReplaceLocal disables local copy prop for
     //    that local (see ReplaceLocal).
 
-    for (GenTreeLclVarCommon* lcl : m_currentStmt->LocalsTreeList())
+    // Normally, we read back only for the uses we will see. However, with
+    // implicit EH flow we may also read back all replacements mid-tree (see
+    // InsertMidTreeReadBacks). So for that case we read back everything. This
+    // is a correctness requirement for QMARKs, but we do it indiscriminately
+    // for the same reasons as mentioned above.
+    if (((m_currentStmt->GetRootNode()->gtFlags & (GTF_EXCEPT | GTF_CALL)) != 0) &&
+        m_compiler->ehBlockHasExnFlowDsc(m_currentBlock))
     {
-        if (lcl->TypeIs(TYP_STRUCT))
-        {
-            continue;
-        }
+        JITDUMP(
+            "Reading back pending replacements before statement with possible exception side effect inside block in try region\n");
 
-        AggregateInfo* agg = m_aggregates.Lookup(lcl->GetLclNum());
-        if (agg == nullptr)
+        for (AggregateInfo* agg : m_aggregates)
         {
-            continue;
+            for (Replacement& rep : agg->Replacements)
+            {
+                InsertPreStatementReadBackIfNecessary(agg->LclNum, rep);
+            }
         }
-
-        size_t index = Promotion::BinarySearch<Replacement, &Replacement::Offset>(agg->Replacements, lcl->GetLclOffs());
-        if ((ssize_t)index < 0)
+    }
+    else
+    {
+        // Otherwise just read back the locals we see uses of.
+        for (GenTreeLclVarCommon* lcl : m_currentStmt->LocalsTreeList())
         {
-            continue;
-        }
+            if (lcl->TypeIs(TYP_STRUCT))
+            {
+                continue;
+            }
 
-        Replacement& rep = agg->Replacements[index];
-        if (rep.NeedsReadBack)
-        {
-            JITDUMP("Reading back replacement V%02u.[%03u..%03u) -> V%02u before [%06u]:\n", agg->LclNum, rep.Offset,
-                    rep.Offset + genTypeSize(rep.AccessType), rep.LclNum,
-                    Compiler::dspTreeID(m_currentStmt->GetRootNode()));
+            AggregateInfo* agg = m_aggregates.Lookup(lcl->GetLclNum());
+            if (agg == nullptr)
+            {
+                continue;
+            }
 
-            GenTree*   readBack = Promotion::CreateReadBack(m_compiler, agg->LclNum, rep);
-            Statement* stmt     = m_compiler->fgNewStmtFromTree(readBack);
-            DISPSTMT(stmt);
-            m_compiler->fgInsertStmtBefore(m_currentBlock, m_currentStmt, stmt);
-            ClearNeedsReadBack(rep);
+            size_t index =
+                Promotion::BinarySearch<Replacement, &Replacement::Offset>(agg->Replacements, lcl->GetLclOffs());
+            if ((ssize_t)index < 0)
+            {
+                continue;
+            }
+
+            InsertPreStatementReadBackIfNecessary(agg->LclNum, agg->Replacements[index]);
         }
     }
 }
 
+//------------------------------------------------------------------------
+// InsertPreStatementReadBackIfNecessary:
+//   Insert a read back of the specified replacement before the current
+//   statement, if the replacement needs it.
+//
+// Parameters:
+//   aggLclNum - Struct local
+//   rep       - The replacement
+//
+void ReplaceVisitor::InsertPreStatementReadBackIfNecessary(unsigned aggLclNum, Replacement& rep)
+{
+    if (!rep.NeedsReadBack)
+    {
+        return;
+    }
+
+    JITDUMP("Reading back replacement V%02u.[%03u..%03u) -> V%02u before [%06u]:\n", aggLclNum, rep.Offset,
+            rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, Compiler::dspTreeID(m_currentStmt->GetRootNode()));
+
+    GenTree*   readBack = Promotion::CreateReadBack(m_compiler, aggLclNum, rep);
+    Statement* stmt     = m_compiler->fgNewStmtFromTree(readBack);
+    DISPSTMT(stmt);
+    m_compiler->fgInsertStmtBefore(m_currentBlock, m_currentStmt, stmt);
+    ClearNeedsReadBack(rep);
+}
+
 //------------------------------------------------------------------------
 // VisitOverlappingReplacements:
 //   Call a function for every replacement that overlaps a specified segment.

src/coreclr/jit/promotion.h

@@ -290,6 +290,7 @@ class ReplaceVisitor : public GenTreeVisitor<ReplaceVisitor>
     bool VisitOverlappingReplacements(unsigned lcl, unsigned offs, unsigned size, Func func);
 
     void      InsertPreStatementReadBacks();
+    void      InsertPreStatementReadBackIfNecessary(unsigned aggLclNum, Replacement& rep);
     void      InsertPreStatementWriteBacks();
     GenTree** InsertMidTreeReadBacks(GenTree** use);
 

src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs

@@ -0,0 +1,55 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.CompilerServices;
+using System.Runtime.Intrinsics;
+using Xunit;
+
+public class Runtime_108969
+{
+    [Fact]
+    public static int TestEntryPoint() => Foo(null);
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static int Foo(object o)
+    {
+        S v = default;
+        try
+        {
+            v = Bar();
+
+            // "(int?)o" creates a QMARK with a branch that may throw; we would
+            // end up reading back v.A inside the QMARK
+            Use((int?)o);
+        }
+        catch (Exception)
+        {
+        }
+
+        // Induce promotion of v.A field
+        Use(v.A);
+        Use(v.A);
+        Use(v.A);
+        Use(v.A);
+        Use(v.A);
+        Use(v.A);
+        return v.A;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static S Bar()
+    {
+        return new S { A = 100 };
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static void Use<T>(T x)
+    {
+    }
+
+    private struct S
+    {
+        public int A, B, C, D;
+    }
+}

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

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