Improvements for dead store removal. (#38004)

1. Don't mark fields of dependently promoted structs as untracked.

2. Remove some stores whose lhs local has a ref count of 1 when running
late liveness. We can rely on ref counts since they are calculated right
before the late liveness pass.

3. Remove dead GT_STORE_BLK in addition to GT_STOREIND in the late
liveness pass.

4. Remove dead stores to untracked locals in the late liveness pass.

5. Allow optRemoveRedundantZeroInits to remove some redundant initializations
of tracked locals. Move the phase to right after liveness so that SSA is correct
after removing assignments to tracked locals.

src/coreclr/src/jit/compiler.cpp

@@ -4757,8 +4757,6 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags
                 DoPhase(this, PHASE_BUILD_SSA, &Compiler::fgSsaBuild);
             }
 
-            DoPhase(this, PHASE_ZERO_INITS, &Compiler::optRemoveRedundantZeroInits);
-
             if (doEarlyProp)
             {
                 // Propagate array length and rewrite getType() method call

src/coreclr/src/jit/lclvars.cpp

@@ -3411,23 +3411,8 @@ void Compiler::lvaSortByRefCount()
                 lvaSetVarDoNotEnregister(lclNum DEBUGARG(DNER_IsStruct));
             }
         }
-        else if (varDsc->lvIsStructField && (lvaGetParentPromotionType(lclNum) != PROMOTION_TYPE_INDEPENDENT) &&
-                 (lvaGetDesc(varDsc->lvParentLcl)->lvRefCnt() > 1))
+        else if (varDsc->lvIsStructField && (lvaGetParentPromotionType(lclNum) != PROMOTION_TYPE_INDEPENDENT))
         {
-            // SSA must exclude struct fields that are not independently promoted
-            // as dependent fields could be assigned using a CopyBlock
-            // resulting in a single node causing multiple SSA definitions
-            // which isn't currently supported by SSA
-            //
-            // If the parent struct local ref count is less than 2, then either the struct is no longer
-            // referenced or the field is no longer referenced: we increment the struct local ref count in incRefCnts
-            // for each field use when the struct is dependently promoted. This can happen, e.g, if we've removed
-            // a block initialization for the struct. In that case we can still track the local field.
-            //
-            // TODO-CQ:  Consider using lvLclBlockOpAddr and only marking these LclVars
-            // untracked when a blockOp is used to assign the struct.
-            //
-            varDsc->lvTracked = 0; // so, don't mark as tracked
             lvaSetVarDoNotEnregister(lclNum DEBUGARG(DNER_DepField));
         }
         else if (varDsc->lvPinned)

src/coreclr/src/jit/liveness.cpp

@@ -1644,6 +1644,33 @@ bool Compiler::fgComputeLifeUntrackedLocal(VARSET_TP&           life,
 {
     assert(lclVarNode != nullptr);
 
+    bool isDef = ((lclVarNode->gtFlags & GTF_VAR_DEF) != 0);
+
+    // We have accurate ref counts when running late liveness so we can eliminate
+    // some stores if the lhs local has a ref count of 1.
+    if (isDef && compRationalIRForm && (varDsc.lvRefCnt() == 1) && !varDsc.lvPinned)
+    {
+        if (varDsc.lvIsStructField)
+        {
+            if ((lvaGetDesc(varDsc.lvParentLcl)->lvRefCnt() == 1) &&
+                (lvaGetParentPromotionType(&varDsc) == PROMOTION_TYPE_DEPENDENT))
+            {
+                return true;
+            }
+        }
+        else if (varTypeIsStruct(varDsc.lvType))
+        {
+            if (lvaGetPromotionType(&varDsc) != PROMOTION_TYPE_INDEPENDENT)
+            {
+                return true;
+            }
+        }
+        else
+        {
+            return true;
+        }
+    }
+
     if (!varTypeIsStruct(varDsc.lvType) || (lvaGetPromotionType(&varDsc) == PROMOTION_TYPE_NONE))
     {
         return false;
@@ -1651,7 +1678,6 @@ bool Compiler::fgComputeLifeUntrackedLocal(VARSET_TP&           life,
 
     VARSET_TP fieldSet(VarSetOps::MakeEmpty(this));
     bool      fieldsAreTracked = true;
-    bool      isDef            = ((lclVarNode->gtFlags & GTF_VAR_DEF) != 0);
 
     for (unsigned i = varDsc.lvFieldLclStart; i < varDsc.lvFieldLclStart + varDsc.lvFieldCnt; ++i)
     {
@@ -1682,8 +1708,12 @@ bool Compiler::fgComputeLifeUntrackedLocal(VARSET_TP&           life,
     if (isDef)
     {
         VARSET_TP liveFields(VarSetOps::Intersection(this, life, fieldSet));
-        VarSetOps::DiffD(this, fieldSet, keepAliveVars);
-        VarSetOps::DiffD(this, life, fieldSet);
+        if ((lclVarNode->gtFlags & GTF_VAR_USEASG) == 0)
+        {
+            VarSetOps::DiffD(this, fieldSet, keepAliveVars);
+            VarSetOps::DiffD(this, life, fieldSet);
+        }
+
         if (fieldsAreTracked && VarSetOps::IsEmpty(this, liveFields))
         {
             // None of the fields were live, so this is a dead store.
@@ -1693,12 +1723,14 @@ bool Compiler::fgComputeLifeUntrackedLocal(VARSET_TP&           life,
                 VARSET_TP keepAliveFields(VarSetOps::Intersection(this, fieldSet, keepAliveVars));
                 noway_assert(VarSetOps::IsEmpty(this, keepAliveFields));
 
-                // Do not consider this store dead if the parent local variable is an address exposed local.
-                return !varDsc.lvAddrExposed;
+                // Do not consider this store dead if the parent local variable is an address exposed local or
+                // if the struct has a custom layout and holes.
+                return !(varDsc.lvAddrExposed || (varDsc.lvCustomLayout && varDsc.lvContainsHoles));
             }
         }
         return false;
     }
+
     // This is a use.
 
     // Are the variables already known to be alive?
@@ -1934,10 +1966,11 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
                     if (isDeadStore)
                     {
                         LIR::Use addrUse;
-                        if (blockRange.TryGetUse(node, &addrUse) && (addrUse.User()->OperGet() == GT_STOREIND))
+                        if (blockRange.TryGetUse(node, &addrUse) &&
+                            (addrUse.User()->OperIs(GT_STOREIND, GT_STORE_BLK, GT_STORE_OBJ)))
                         {
                             // Remove the store. DCE will iteratively clean up any ununsed operands.
-                            GenTreeStoreInd* const store = addrUse.User()->AsStoreInd();
+                            GenTreeIndir* const store = addrUse.User()->AsIndir();
 
                             JITDUMP("Removing dead indirect store:\n");
                             DISPNODE(store);
@@ -1945,7 +1978,15 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
                             assert(store->Addr() == node);
                             blockRange.Delete(this, block, node);
 
-                            store->Data()->SetUnusedValue();
+                            GenTree* data =
+                                store->OperIs(GT_STOREIND) ? store->AsStoreInd()->Data() : store->AsBlk()->Data();
+                            data->SetUnusedValue();
+                            if (data->isIndir())
+                            {
+                                // This is a block assignment. An indirection of the rhs is not considered
+                                // to happen until the assignment so mark it as non-faulting.
+                                data->gtFlags |= GTF_IND_NONFAULTING;
+                            }
 
                             blockRange.Remove(store);
 
@@ -1965,39 +2006,41 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
                 if (varDsc.lvTracked)
                 {
                     isDeadStore = fgComputeLifeTrackedLocalDef(life, keepAliveVars, varDsc, lclVarNode);
-                    if (isDeadStore)
-                    {
-                        JITDUMP("Removing dead store:\n");
-                        DISPNODE(lclVarNode);
+                }
+                else
+                {
+                    isDeadStore = fgComputeLifeUntrackedLocal(life, keepAliveVars, varDsc, lclVarNode);
+                }
 
-                        // Remove the store. DCE will iteratively clean up any ununsed operands.
-                        lclVarNode->gtOp1->SetUnusedValue();
+                if (isDeadStore)
+                {
+                    JITDUMP("Removing dead store:\n");
+                    DISPNODE(lclVarNode);
 
-                        // If the store is marked as a late argument, it is referenced by a call. Instead of removing
-                        // it, bash it to a NOP.
-                        if ((node->gtFlags & GTF_LATE_ARG) != 0)
-                        {
-                            JITDUMP("node is a late arg; replacing with NOP\n");
-                            node->gtBashToNOP();
+                    // Remove the store. DCE will iteratively clean up any ununsed operands.
+                    lclVarNode->gtOp1->SetUnusedValue();
 
-                            // NOTE: this is a bit of a hack. We need to keep these nodes around as they are
-                            // referenced by the call, but they're considered side-effect-free non-value-producing
-                            // nodes, so they will be removed if we don't do this.
-                            node->gtFlags |= GTF_ORDER_SIDEEFF;
-                        }
-                        else
-                        {
-                            blockRange.Remove(node);
-                        }
+                    // If the store is marked as a late argument, it is referenced by a call. Instead of removing
+                    // it, bash it to a NOP.
+                    if ((node->gtFlags & GTF_LATE_ARG) != 0)
+                    {
+                        JITDUMP("node is a late arg; replacing with NOP\n");
+                        node->gtBashToNOP();
 
-                        assert(!opts.MinOpts());
-                        fgStmtRemoved = true;
+                        // NOTE: this is a bit of a hack. We need to keep these nodes around as they are
+                        // referenced by the call, but they're considered side-effect-free non-value-producing
+                        // nodes, so they will be removed if we don't do this.
+                        node->gtFlags |= GTF_ORDER_SIDEEFF;
                     }
+                    else
+                    {
+                        blockRange.Remove(node);
+                    }
+
+                    assert(!opts.MinOpts());
+                    fgStmtRemoved = true;
                 }
-                else
-                {
-                    fgComputeLifeUntrackedLocal(life, keepAliveVars, varDsc, lclVarNode);
-                }
+
                 break;
             }
 
@@ -2533,7 +2576,7 @@ void Compiler::fgInterBlockLocalVarLiveness()
             if (isFinallyVar)
             {
                 // Set lvMustInit only if we have a non-arg, GC pointer.
-                if (!varDsc->lvIsParam && varTypeIsGC(varDsc->TypeGet()) && !fieldOfDependentlyPromotedStruct)
+                if (!varDsc->lvIsParam && varTypeIsGC(varDsc->TypeGet()))
                 {
                     varDsc->lvMustInit = true;
                 }

src/coreclr/src/jit/optimizer.cpp

@@ -9241,6 +9241,9 @@ void Compiler::optRemoveRedundantZeroInits()
          block             = block->GetUniqueSucc())
     {
         block->bbFlags |= BBF_MARKED;
+        CompAllocator   allocator(getAllocator(CMK_ZeroInit));
+        LclVarRefCounts defsInBlock(allocator);
+        bool            removedTrackedDefs = false;
         for (Statement* stmt = block->FirstNonPhiDef(); stmt != nullptr;)
         {
             Statement* next = stmt->GetNextStmt();
@@ -9274,6 +9277,48 @@ void Compiler::optRemoveRedundantZeroInits()
                             refCounts.Set(lclNum, 1);
                         }
 
+                        if ((tree->gtFlags & GTF_VAR_DEF) == 0)
+                        {
+                            break;
+                        }
+
+                        // We need to count the number of tracked var defs in the block
+                        // so that we can update block->bbVarDef if we remove any tracked var defs.
+
+                        LclVarDsc* const lclDsc = lvaGetDesc(lclNum);
+                        if (lclDsc->lvTracked)
+                        {
+                            unsigned* pDefsCount = defsInBlock.LookupPointer(lclNum);
+                            if (pDefsCount != nullptr)
+                            {
+                                *pDefsCount = (*pDefsCount) + 1;
+                            }
+                            else
+                            {
+                                defsInBlock.Set(lclNum, 1);
+                            }
+                        }
+                        else if (varTypeIsStruct(lclDsc) && ((tree->gtFlags & GTF_VAR_USEASG) == 0) &&
+                                 lvaGetPromotionType(lclDsc) != PROMOTION_TYPE_NONE)
+                        {
+                            for (unsigned i = lclDsc->lvFieldLclStart; i < lclDsc->lvFieldLclStart + lclDsc->lvFieldCnt;
+                                 ++i)
+                            {
+                                if (lvaGetDesc(i)->lvTracked)
+                                {
+                                    unsigned* pDefsCount = defsInBlock.LookupPointer(i);
+                                    if (pDefsCount != nullptr)
+                                    {
+                                        *pDefsCount = (*pDefsCount) + 1;
+                                    }
+                                    else
+                                    {
+                                        defsInBlock.Set(i, 1);
+                                    }
+                                }
+                            }
+                        }
+
                         break;
                     }
                     case GT_ASG:
@@ -9339,26 +9384,29 @@ void Compiler::optRemoveRedundantZeroInits()
 
                         if (treeOp->gtOp2->IsIntegralConst(0))
                         {
-                            if (!lclDsc->lvTracked)
-                            {
-                                bool bbInALoop  = (block->bbFlags & BBF_BACKWARD_JUMP) != 0;
-                                bool bbIsReturn = block->bbJumpKind == BBJ_RETURN;
+                            bool bbInALoop  = (block->bbFlags & BBF_BACKWARD_JUMP) != 0;
+                            bool bbIsReturn = block->bbJumpKind == BBJ_RETURN;
 
-                                if (BitVecOps::IsMember(&bitVecTraits, zeroInitLocals, lclNum) ||
-                                    (lclDsc->lvIsStructField &&
-                                     BitVecOps::IsMember(&bitVecTraits, zeroInitLocals, lclDsc->lvParentLcl)) ||
-                                    !fgVarNeedsExplicitZeroInit(lclNum, bbInALoop, bbIsReturn))
+                            if (BitVecOps::IsMember(&bitVecTraits, zeroInitLocals, lclNum) ||
+                                (lclDsc->lvIsStructField &&
+                                 BitVecOps::IsMember(&bitVecTraits, zeroInitLocals, lclDsc->lvParentLcl)) ||
+                                (!lclDsc->lvTracked && !fgVarNeedsExplicitZeroInit(lclNum, bbInALoop, bbIsReturn)))
+                            {
+                                // We are guaranteed to have a zero initialization in the prolog or a
+                                // dominating explicit zero initialization and the local hasn't been redefined
+                                // between the prolog and this explicit zero initialization so the assignment
+                                // can be safely removed.
+                                if (tree == stmt->GetRootNode())
                                 {
-                                    // We are guaranteed to have a zero initialization in the prolog or a
-                                    // dominating explicit zero initialization and the local hasn't been redefined
-                                    // between the prolog and this explicit zero initialization so the assignment
-                                    // can be safely removed.
-                                    if (tree == stmt->GetRootNode())
+                                    fgRemoveStmt(block, stmt);
+                                    removedExplicitZeroInit      = true;
+                                    lclDsc->lvSuppressedZeroInit = 1;
+
+                                    if (lclDsc->lvTracked)
                                     {
-                                        fgRemoveStmt(block, stmt);
-                                        removedExplicitZeroInit      = true;
-                                        lclDsc->lvSuppressedZeroInit = 1;
-                                        lclDsc->setLvRefCnt(lclDsc->lvRefCnt() - 1);
+                                        removedTrackedDefs   = true;
+                                        unsigned* pDefsCount = defsInBlock.LookupPointer(lclNum);
+                                        *pDefsCount          = (*pDefsCount) - 1;
                                     }
                                 }
                             }
@@ -9392,6 +9440,20 @@ void Compiler::optRemoveRedundantZeroInits()
             }
             stmt = next;
         }
+
+        if (removedTrackedDefs)
+        {
+            LclVarRefCounts::KeyIterator iter(defsInBlock.Begin());
+            LclVarRefCounts::KeyIterator end(defsInBlock.End());
+            for (; !iter.Equal(end); iter++)
+            {
+                unsigned int lclNum = iter.Get();
+                if (defsInBlock[lclNum] == 0)
+                {
+                    VarSetOps::RemoveElemD(this, block->bbVarDef, lvaGetDesc(lclNum)->lvVarIndex);
+                }
+            }
+        }
     }
 
     for (BasicBlock* block = fgFirstBB; (block != nullptr) && ((block->bbFlags & BBF_MARKED) != 0);

src/coreclr/src/jit/ssabuilder.cpp

@@ -1542,6 +1542,9 @@ void SsaBuilder::Build()
     m_pCompiler->fgLocalVarLiveness();
     EndPhase(PHASE_BUILD_SSA_LIVENESS);
 
+    m_pCompiler->optRemoveRedundantZeroInits();
+    EndPhase(PHASE_ZERO_INITS);
+
     // Mark all variables that will be tracked by SSA
     for (unsigned lclNum = 0; lclNum < m_pCompiler->lvaCount; lclNum++)
     {