JIT: Fix block morphing/physical promotion self-interference (#85887)

Fix a couple of cases where it's possible that the decomposed stores
will modify the address being used as part of the operation too early.
In those cases we must spill the address to a new local ahead of time.

Fix #85874

src/coreclr/jit/morphblock.cpp

@@ -662,6 +662,9 @@ class MorphCopyBlockHelper : public MorphInitBlockHelper
 
     bool m_dstDoFldAsg = false;
     bool m_srcDoFldAsg = false;
+
+private:
+    bool CanReuseAddressForDecomposedStore(GenTree* addr);
 };
 
 //------------------------------------------------------------------------
@@ -1170,7 +1173,7 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField()
             // and spill, unless we only end up using the address once.
             if (fieldCnt - dyingFieldCnt > 1)
             {
-                if (m_comp->gtClone(srcAddr))
+                if (CanReuseAddressForDecomposedStore(srcAddr))
                 {
                     // "srcAddr" is simple expression. No need to spill.
                     noway_assert((srcAddr->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) == 0);
@@ -1202,7 +1205,7 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField()
             // and spill, unless we only end up using the address once.
             if (m_srcVarDsc->lvFieldCnt > 1)
             {
-                if (m_comp->gtClone(dstAddr))
+                if (CanReuseAddressForDecomposedStore(dstAddr))
                 {
                     // "dstAddr" is simple expression. No need to spill
                     noway_assert((dstAddr->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) == 0);
@@ -1512,6 +1515,63 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField()
     return result;
 }
 
+//------------------------------------------------------------------------
+// CanReuseAddressForDecomposedStore: Check if it is safe to reuse the
+// specified address node for each decomposed store of a block copy.
+//
+// Arguments:
+//   addrNode - The address node
+//
+// Return Value:
+//   True if the caller can reuse the address by cloning.
+//
+bool MorphCopyBlockHelper::CanReuseAddressForDecomposedStore(GenTree* addrNode)
+{
+    if (addrNode->OperIsLocalRead())
+    {
+        GenTreeLclVarCommon* lcl    = addrNode->AsLclVarCommon();
+        unsigned             lclNum = lcl->GetLclNum();
+        LclVarDsc*           lclDsc = m_comp->lvaGetDesc(lclNum);
+        if (lclDsc->IsAddressExposed())
+        {
+            // Address could be pointing to itself
+            return false;
+        }
+
+        // The store can also directly write to the address. For example:
+        //
+        //  ▌  STORE_LCL_VAR struct<Program+ListElement, 16>(P) V00 loc0
+        //  ▌    long   V00.Program+ListElement:Next (offs=0x00) -> V03 tmp1
+        //  ▌    int    V00.Program+ListElement:Value (offs=0x08) -> V04 tmp2
+        //  └──▌  BLK       struct<Program+ListElement, 16>
+        //     └──▌  LCL_VAR   long   V03 tmp1          (last use)
+        //
+        // If we reused the address we would produce
+        //
+        //  ▌  COMMA     void
+        //  ├──▌  STORE_LCL_VAR long   V03 tmp1
+        //  │  └──▌  IND       long
+        //  │     └──▌  LCL_VAR   long   V03 tmp1          (last use)
+        //  └──▌  STORE_LCL_VAR int    V04 tmp2
+        //     └──▌  IND       int
+        //        └──▌  ADD       byref
+        //           ├──▌  LCL_VAR   long   V03 tmp1          (last use)
+        //           └──▌  CNS_INT   long   8
+        //
+        // which is also obviously incorrect.
+        //
+
+        if (m_dstLclNum == BAD_VAR_NUM)
+        {
+            return true;
+        }
+
+        return (lclNum != m_dstLclNum) && (!lclDsc->lvIsStructField || (lclDsc->lvParentLcl != m_dstLclNum));
+    }
+
+    return addrNode->IsInvariant();
+}
+
 //------------------------------------------------------------------------
 // fgMorphCopyBlock: Perform the morphing of a block copy.
 //

src/coreclr/jit/promotiondecomposition.cpp

@@ -814,7 +814,6 @@ class DecompositionPlan
             for (int i = 0; i < m_entries.Height(); i++)
             {
                 const Entry& entry = m_entries.BottomRef(i);
-                // TODO: Double check that TYP_BYREF do not incur any write barriers.
                 if ((entry.FromReplacement != nullptr) && (entry.Type == TYP_REF))
                 {
                     Replacement* rep = entry.FromReplacement;
@@ -901,16 +900,14 @@ class DecompositionPlan
 
         if ((addr != nullptr) && (numAddrUses > 1))
         {
-            if (addr->OperIsLocal() && (!m_dst->OperIs(GT_LCL_VAR, GT_LCL_FLD) ||
-                                        (addr->AsLclVarCommon()->GetLclNum() != m_dst->AsLclVarCommon()->GetLclNum())))
+            if (CanReuseAddressForDecomposedStore(addr))
             {
-                // We will introduce more uses of the address local, so it is
-                // no longer dying here.
-                addr->gtFlags &= ~GTF_VAR_DEATH;
-            }
-            else if (addr->IsInvariant())
-            {
-                // Fall through
+                if (addr->OperIsLocalRead())
+                {
+                    // We will introduce more uses of the address local, so it is
+                    // no longer dying here.
+                    addr->gtFlags &= ~GTF_VAR_DEATH;
+                }
             }
             else
             {
@@ -1104,6 +1101,58 @@ class DecompositionPlan
                !entry.FromReplacement->NeedsWriteBack && (entry.ToLclNum == BAD_VAR_NUM);
     }
 
+    //------------------------------------------------------------------------
+    // CanReuseAddressForDecomposedStore: Check if it is safe to reuse the
+    // specified address node for each decomposed store of a block copy.
+    //
+    // Arguments:
+    //   addrNode - The address node
+    //
+    // Return Value:
+    //   True if the caller can reuse the address by cloning.
+    //
+    bool CanReuseAddressForDecomposedStore(GenTree* addrNode)
+    {
+        if (addrNode->OperIsLocalRead())
+        {
+            GenTreeLclVarCommon* lcl    = addrNode->AsLclVarCommon();
+            unsigned             lclNum = lcl->GetLclNum();
+            if (m_compiler->lvaGetDesc(lclNum)->IsAddressExposed())
+            {
+                // Address could be pointing to itself
+                return false;
+            }
+
+            // If we aren't writing a local here then since the address is not
+            // exposed it cannot change.
+            if (!m_dst->OperIs(GT_LCL_VAR, GT_LCL_FLD))
+            {
+                return true;
+            }
+
+            // Otherwise it could still be possible that the address is part of
+            // the struct we're writing.
+            unsigned dstLclNum = m_dst->AsLclVarCommon()->GetLclNum();
+            if (lclNum == dstLclNum)
+            {
+                return false;
+            }
+
+            // It could also be one of the replacement locals we're going to write.
+            for (int i = 0; i < m_entries.Height(); i++)
+            {
+                if (m_entries.BottomRef(i).ToLclNum == lclNum)
+                {
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
+        return addrNode->IsInvariant();
+    }
+
     //------------------------------------------------------------------------
     // PropagateIndirFlags:
     //   Propagate the specified flags to a GT_IND node.

src/tests/JIT/Directed/physicalpromotion/addressinterference.cs

@@ -0,0 +1,47 @@
+// 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 Xunit;
+
+public unsafe class PhysicalPromotionAddressInterference
+{
+    [Fact]
+    public static int Exposed()
+    {
+        ListElement e1;
+        e1.Next = &e1;
+        ListElement e2;
+        e2.Next = null;
+        e2.Value = 100;
+        *e1.Next = e2;
+        return e1.Value;
+    }
+
+    [Fact]
+    public static int DestinationIsAddress()
+    {
+        ListElement e1;
+        ListElement e2 = default;
+        e2.Value = 100;
+        e1.Next = &e2;
+        e1.Value = 1234;
+        Consume(e1);
+        e1 = *e1.Next;
+        Consume(e1);
+        return e1.Value;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void Consume<T>(T val)
+    {
+    }
+
+    struct ListElement
+    {
+        public ListElement* Next;
+        public int Value;
+    }
+
+}

src/tests/JIT/Directed/physicalpromotion/addressinterference.csproj

@@ -0,0 +1,10 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <Optimize>True</Optimize>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+    <CLRTestEnvironmentVariable Include="DOTNET_JitStressModeNames" Value="STRESS_PHYSICAL_PROMOTION STRESS_PHYSICAL_PROMOTION_COST STRESS_NO_OLD_PROMOTION" />
+  </ItemGroup>
+</Project>

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

@@ -0,0 +1,47 @@
+// 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 Xunit;
+
+public unsafe class Runtime_85874
+{
+    [Fact]
+    public static int Exposed()
+    {
+        ListElement e1;
+        e1.Next = &e1;
+        ListElement e2;
+        e2.Next = null;
+        e2.Value = 100;
+        *e1.Next = e2;
+        return e1.Value;
+    }
+
+    [Fact]
+    public static int DestinationIsAddress()
+    {
+        ListElement e1;
+        ListElement e2 = default;
+        e2.Value = 100;
+        e1.Next = &e2;
+        e1.Value = 1234;
+        Consume(e1);
+        e1 = *e1.Next;
+        Consume(e1);
+        return e1.Value;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void Consume<T>(T val)
+    {
+    }
+
+    struct ListElement
+    {
+        public ListElement* Next;
+        public int Value;
+    }
+
+}

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

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