Expand the GC hole fix for explicitly initialized structs (#69501)

* Expand the fix for explicitly initialized structs

Liveness code already recognizes that it cannot delete certain local
stores with the implicit side effect of "explicit initialization".

However, it was only doing that for indirect "InitBlk" forms, while
the store can have more or less arbitrary shape (the only requirement
is that is must be "entire").

Fix this by not constraining the check to "InitBlk"s.

* Add a test

Author: SingleAccretion

src/coreclr/jit/compiler.h

@@ -4591,7 +4591,8 @@ class Compiler
 
     bool fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange);
 
-    void fgRemoveDeadStoreLIR(GenTree* store, BasicBlock* block);
+    bool fgTryRemoveDeadStoreLIR(GenTree* store, GenTreeLclVarCommon* lclNode, BasicBlock* block);
+
     bool fgRemoveDeadStore(GenTree**        pTree,
                            LclVarDsc*       varDsc,
                            VARSET_VALARG_TP life,

src/coreclr/jit/gentree.h

@@ -3280,6 +3280,12 @@ struct GenTreeLclVarCommon : public GenTreeUnOp
         SetLclNum(lclNum);
     }
 
+    GenTree*& Data()
+    {
+        assert(OperIsLocalStore());
+        return gtOp1;
+    }
+
     unsigned GetLclNum() const
     {
         return _gtLclNum;
@@ -6602,6 +6608,12 @@ struct GenTreeIndir : public GenTreeOp
         gtOp1 = addr;
     }
 
+    GenTree*& Data()
+    {
+        assert(OperIs(GT_STOREIND) || OperIsStoreBlk());
+        return gtOp2;
+    }
+
     // these methods provide an interface to the indirection node which
     bool     HasBase();
     bool     HasIndex();

src/coreclr/jit/liveness.cpp

@@ -1970,42 +1970,19 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
                         {
                             GenTreeIndir* const store = addrUse.User()->AsIndir();
 
-                            // If this is a zero init of an explicit zero init gc local
-                            // that has at least one other reference, we will keep the zero init.
-                            //
-                            const LclVarDsc& varDsc              = lvaTable[node->AsLclVarCommon()->GetLclNum()];
-                            const bool       isExplicitInitLocal = varDsc.lvHasExplicitInit;
-                            const bool       isReferencedLocal   = varDsc.lvRefCnt() > 1;
-                            const bool       isZeroInit          = store->OperIsInitBlkOp();
-                            const bool       isGCInit            = varDsc.HasGCPtr();
-
-                            if (isExplicitInitLocal && isReferencedLocal && isZeroInit && isGCInit)
+                            if (fgTryRemoveDeadStoreLIR(store, node->AsLclVarCommon(), block))
                             {
-                                // Yes, we'd better keep it around.
-                                //
-                                JITDUMP("Keeping dead indirect store -- explicit zero init of gc type\n");
-                                DISPNODE(store);
-                            }
-                            else
-                            {
-                                // Remove the store. DCE will iteratively clean up any ununsed operands.
-                                //
-                                JITDUMP("Removing dead indirect store:\n");
-                                DISPNODE(store);
-
-                                assert(store->Addr() == node);
-                                blockRange.Delete(this, block, node);
+                                JITDUMP("Removing dead LclVar address:\n");
+                                DISPNODE(node);
+                                blockRange.Remove(node);
 
-                                GenTree* data =
-                                    store->OperIs(GT_STOREIND) ? store->AsStoreInd()->Data() : store->AsBlk()->Data();
+                                GenTree* data = store->AsIndir()->Data();
                                 data->SetUnusedValue();
 
                                 if (data->isIndir())
                                 {
                                     Lowering::TransformUnusedIndirection(data->AsIndir(), this, block);
                                 }
-
-                                fgRemoveDeadStoreLIR(store, block);
                             }
                         }
                     }
@@ -2027,17 +2004,10 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
                     isDeadStore = fgComputeLifeUntrackedLocal(life, keepAliveVars, varDsc, lclVarNode);
                 }
 
-                if (isDeadStore)
+                if (isDeadStore && fgTryRemoveDeadStoreLIR(node, lclVarNode, block))
                 {
-                    JITDUMP("Removing dead store:\n");
-                    DISPNODE(lclVarNode);
-
-                    // Remove the store. DCE will iteratively clean up any ununsed operands.
-                    lclVarNode->gtOp1->SetUnusedValue();
-
-                    fgRemoveDeadStoreLIR(node, block);
+                    lclVarNode->Data()->SetUnusedValue();
                 }
-
                 break;
             }
 
@@ -2185,17 +2155,41 @@ bool Compiler::fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange)
 }
 
 //---------------------------------------------------------------------
-// fgRemoveDeadStoreLIR - remove a dead store from LIR
+// fgTryRemoveDeadStoreLIR - try to remove a dead store from LIR
 //
-//   store          - A store tree
-//   block          - Block that the store is part of
+// Arguments:
+//   store   - A store tree
+//   lclNode - The node representing the local being stored to
+//   block   - Block that the store is part of
+//
+// Return Value:
+//    Whether the store was successfully removed from "block"'s range.
 //
-void Compiler::fgRemoveDeadStoreLIR(GenTree* store, BasicBlock* block)
+bool Compiler::fgTryRemoveDeadStoreLIR(GenTree* store, GenTreeLclVarCommon* lclNode, BasicBlock* block)
 {
-    LIR::Range& blockRange = LIR::AsRange(block);
-    blockRange.Remove(store);
     assert(!opts.MinOpts());
+
+    // We cannot remove stores to (tracked) TYP_STRUCT locals with GC pointers marked as "explicit init",
+    // as said locals will be reported to the GC untracked, and deleting the explicit initializer risks
+    // exposing uninitialized references.
+    if ((lclNode->gtFlags & GTF_VAR_USEASG) == 0)
+    {
+        LclVarDsc* varDsc = lvaGetDesc(lclNode);
+        if (varDsc->lvHasExplicitInit && (varDsc->TypeGet() == TYP_STRUCT) && varDsc->HasGCPtr() &&
+            (varDsc->lvRefCnt() > 1))
+        {
+            JITDUMP("Not removing a potential explicit init [%06u] of V%02u\n", dspTreeID(store), lclNode->GetLclNum());
+            return false;
+        }
+    }
+
+    JITDUMP("Removing dead %s:\n", store->OperIsIndir() ? "indirect store" : "local store");
+    DISPNODE(store);
+
+    LIR::AsRange(block).Remove(store);
     fgStmtRemoved = true;
+
+    return true;
 }
 
 //---------------------------------------------------------------------

src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694_2.cs

@@ -0,0 +1,41 @@
+// 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;
+
+public class Runtime_65694_2
+{
+    public static int Main()
+    {
+        var a = new StructWithObj { Obj = new object() };
+        var c = new StructWithObj { Obj = new object() };
+
+        return Problem(a, c).Obj == c.Obj ? 100 : 101;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static StructWithObj Problem(StructWithObj a, StructWithObj c)
+    {
+        StructWithObj b = a;
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static void GcSafePoint() { GC.Collect(); }
+
+        GcSafePoint();
+        GcSafePoint();
+
+        if (a.Obj == b.Obj)
+        {
+            b = c;
+        }
+
+        return b;
+    }
+
+    struct StructWithObj
+    {
+        public object Obj;
+    }
+}
+

src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694_2.csproj

@@ -0,0 +1,17 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <Optimize>True</Optimize>
+    <CLRTestBatchPreCommands><![CDATA[
+      $(CLRTestBatchPreCommands)
+      set DOTNET_JitNoStructPromotion=1
+      ]]></CLRTestBatchPreCommands>
+    <BashCLRTestPreCommands><![CDATA[
+      $(BashCLRTestPreCommands)
+      export DOTNET_JitNoStructPromotion=1
+      ]]></BashCLRTestPreCommands>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>