JIT: Handle promoted struct liveness updates in forward sub (#81789)

This logic needed to be updated together with 7131ef9, without the
update we have a correctness bug.

The new correct and more precise unmarking also allows us to remove a
case from fgMakeOutgoingStructArgCopy. The case is covered by the
last-use optimization one now (specifically it was only happening for
some undone promotion cases).

diff --git a/src/coreclr/jit/forwardsub.cpp b/src/coreclr/jit/forwardsub.cpp
index 0cd3871..5099831 100644 (file)
--- a/src/coreclr/jit/forwardsub.cpp
+++ b/src/coreclr/jit/forwardsub.cpp
@@ -912,33 +912,49 @@ void Compiler::fgForwardSubUpdateLiveness(GenTree* newSubListFirst, GenTree* new
 {
     for (GenTree* node = newSubListFirst->gtPrev; node != nullptr; node = node->gtPrev)
     {
-        if ((node->gtFlags & GTF_VAR_DEATH) == 0)
+        if ((node->gtFlags & GTF_VAR_DEATH_MASK) == 0)
         {
             continue;
         }
 
         unsigned   lclNum = node->AsLclVarCommon()->GetLclNum();
         LclVarDsc* dsc    = lvaGetDesc(lclNum);
-        // Last-use copy omission does not work for promoted structs today, so
-        // we can always unmark these which saves us from having to update the
-        // promoted struct death vars map.
-        if (dsc->lvPromoted)
-        {
-            node->gtFlags &= ~GTF_VAR_DEATH;
-            continue;
-        }
 
         unsigned parentLclNum = dsc->lvIsStructField ? dsc->lvParentLcl : BAD_VAR_NUM;
 
         GenTree* candidate = newSubListFirst;
-        // See if a new instance of this local or its parent appeared.
         while (true)
         {
             unsigned newUseLclNum = candidate->AsLclVarCommon()->GetLclNum();
-            if ((newUseLclNum == lclNum) || (newUseLclNum == parentLclNum))
+            if (dsc->lvPromoted)
             {
-                node->gtFlags &= ~GTF_VAR_DEATH;
-                break;
+                // Is the parent struct being used?
+                if (newUseLclNum == lclNum)
+                {
+                    // Then all fields are not dying.
+                    node->gtFlags &= ~GTF_VAR_DEATH_MASK;
+                    break;
+                }
+
+                // Otherwise, is one single field being used?
+                if ((newUseLclNum >= dsc->lvFieldLclStart) && (newUseLclNum < dsc->lvFieldLclStart + dsc->lvFieldCnt))
+                {
+                    node->ClearLastUse(newUseLclNum - dsc->lvFieldLclStart);
+
+                    if ((node->gtFlags & GTF_VAR_DEATH_MASK) == 0)
+                    {
+                        break;
+                    }
+                }
+            }
+            else
+            {
+                // See if a new instance of this local or its parent appeared.
+                if ((newUseLclNum == lclNum) || (newUseLclNum == parentLclNum))
+                {
+                    node->gtFlags &= ~GTF_VAR_DEATH;
+                    break;
+                }
             }
 
             if (candidate == newSubListLast)

diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h
index 1e3f872..0e19fd8 100644 (file)
--- a/src/coreclr/jit/gentree.h
+++ b/src/coreclr/jit/gentree.h
@@ -9318,21 +9318,21 @@ inline void GenTree::SetRegSpillFlagByIdx(GenTreeFlags flags, int regIndex)
 // GetLastUseBit: Get the last use bit for regIndex
 //
 // Arguments:
-//     regIndex - the register index
+//     fieldIndex - the field index
 //
 // Return Value:
-//     The bit to set, clear or query for the last-use of the regIndex'th value.
+//     The bit to set, clear or query for the last-use of the fieldIndex'th value.
 //
 // Notes:
 //     This must be a GenTreeLclVar or GenTreeCopyOrReload node.
 //
-inline GenTreeFlags GenTree::GetLastUseBit(int regIndex) const
+inline GenTreeFlags GenTree::GetLastUseBit(int fieldIndex) const
 {
-    assert(regIndex < 4);
+    assert(fieldIndex < 4);
     assert(OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_LCL_VAR_ADDR, GT_LCL_FLD, GT_STORE_LCL_FLD, GT_LCL_FLD_ADDR, GT_COPY,
                   GT_RELOAD));
     static_assert_no_msg((1 << FIELD_LAST_USE_SHIFT) == GTF_VAR_FIELD_DEATH0);
-    return (GenTreeFlags)(1 << (FIELD_LAST_USE_SHIFT + regIndex));
+    return (GenTreeFlags)(1 << (FIELD_LAST_USE_SHIFT + fieldIndex));
 }
 
 //-----------------------------------------------------------------------------------
@@ -9365,6 +9365,8 @@ inline bool GenTree::IsLastUse(int fieldIndex) const
 //
 inline bool GenTree::HasLastUse() const
 {
+    assert(OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_LCL_VAR_ADDR, GT_LCL_FLD, GT_STORE_LCL_FLD, GT_LCL_FLD_ADDR, GT_COPY,
+                  GT_RELOAD));
     return (gtFlags & (GTF_VAR_DEATH_MASK)) != 0;
 }
 
@@ -9372,28 +9374,28 @@ inline bool GenTree::HasLastUse() const
 // SetLastUse: Set the last use bit for the given index
 //
 // Arguments:
-//     regIndex - the index
+//     fieldIndex - the index
 //
 // Notes:
 //     This must be a GenTreeLclVar or GenTreeCopyOrReload node.
 //
-inline void GenTree::SetLastUse(int index)
+inline void GenTree::SetLastUse(int fieldIndex)
 {
-    gtFlags |= GetLastUseBit(index);
+    gtFlags |= GetLastUseBit(fieldIndex);
 }
 
 //-----------------------------------------------------------------------------------
 // ClearLastUse: Clear the last use bit for the given index
 //
 // Arguments:
-//     regIndex - the register index
+//     fieldIndex - the index
 //
 // Notes:
 //     This must be a GenTreeLclVar or GenTreeCopyOrReload node.
 //
-inline void GenTree::ClearLastUse(int regIndex)
+inline void GenTree::ClearLastUse(int fieldIndex)
 {
-    gtFlags &= ~GetLastUseBit(regIndex);
+    gtFlags &= ~GetLastUseBit(fieldIndex);
 }
 
 //-------------------------------------------------------------------------

diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp
index 7216b77..a34552a 100644 (file)
--- a/src/coreclr/jit/morph.cpp
+++ b/src/coreclr/jit/morph.cpp
@@ -3979,12 +3979,6 @@ void Compiler::fgMakeOutgoingStructArgCopy(GenTreeCall* call, CallArg* arg)
                 omitCopy = !varDsc->lvPromoted && !varDsc->lvIsStructField && ((lcl->gtFlags & GTF_VAR_DEATH) != 0);
             }
 
-            if (!omitCopy && (totalAppearances == 1))
-            {
-                // fgMightHaveLoop() is expensive; check it last, only if necessary.
-                omitCopy = call->IsNoReturn() || !fgMightHaveLoop();
-            }
-
             if (omitCopy)
             {
                 if (implicitByRefLcl != nullptr)