JIT: Reorder physical promotion and forward sub (#87265)

Physical promotion breaks more forward sub opportunities than it
introduces, so reorder these phases. Then teach physical promotion to
recognize when it creates new opportunities and selectively reinvoke
forward sub for just those cases.

Author: jakobbotsch

src/coreclr/jit/compiler.cpp

@@ -4716,14 +4716,14 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
     //
     DoPhase(this, PHASE_EARLY_LIVENESS, &Compiler::fgEarlyLiveness);
 
-    // Promote struct locals based on primitive access patterns
-    //
-    DoPhase(this, PHASE_PHYSICAL_PROMOTION, &Compiler::PhysicalPromotion);
-
     // Run a simple forward substitution pass.
     //
     DoPhase(this, PHASE_FWD_SUB, &Compiler::fgForwardSub);
 
+    // Promote struct locals based on primitive access patterns
+    //
+    DoPhase(this, PHASE_PHYSICAL_PROMOTION, &Compiler::PhysicalPromotion);
+
     // Locals tree list is no longer kept valid.
     fgNodeThreading = NodeThreading::None;
 

src/coreclr/jit/promotion.cpp

@@ -1358,6 +1358,7 @@ void ReplaceVisitor::LoadStoreAroundCall(GenTreeCall* call, GenTree* user)
             if ((m_aggregates[argNodeLcl->GetLclNum()] != nullptr) && IsPromotedStructLocalDying(argNodeLcl))
             {
                 argNodeLcl->gtFlags |= GTF_VAR_DEATH;
+                CheckForwardSubForLastUse(argNodeLcl->GetLclNum());
             }
         }
     }
@@ -1498,7 +1499,11 @@ void ReplaceVisitor::ReplaceLocal(GenTree** use, GenTree* user)
         *use = m_compiler->gtNewLclvNode(rep.LclNum, accessType);
     }
 
-    (*use)->gtFlags |= lcl->gtFlags & GTF_VAR_DEATH;
+    if ((lcl->gtFlags & GTF_VAR_DEATH) != 0)
+    {
+        (*use)->gtFlags |= GTF_VAR_DEATH;
+        CheckForwardSubForLastUse(rep.LclNum);
+    }
 
     if (isDef)
     {
@@ -1540,6 +1545,30 @@ void ReplaceVisitor::ReplaceLocal(GenTree** use, GenTree* user)
     m_madeChanges = true;
 }
 
+//------------------------------------------------------------------------
+// CheckForwardSubForLastUse:
+//   Indicate that a local has a last use in the current statement and that
+//   there thus may be a forward substitution opportunity.
+//
+// Parameters:
+//   lclNum - The local number with a last use in this statement.
+//
+void ReplaceVisitor::CheckForwardSubForLastUse(unsigned lclNum)
+{
+    if (m_currentBlock->firstStmt() == m_currentStmt)
+    {
+        return;
+    }
+
+    Statement* prevStmt = m_currentStmt->GetPrevStmt();
+    GenTree*   prevNode = prevStmt->GetRootNode();
+
+    if (prevNode->OperIsLocalStore() && (prevNode->AsLclVarCommon()->GetLclNum() == lclNum))
+    {
+        m_mayHaveForwardSub = true;
+    }
+}
+
 //------------------------------------------------------------------------
 // StoreBeforeReturn:
 //   Handle a return of a potential struct local.
@@ -1780,7 +1809,7 @@ PhaseStatus Promotion::Run()
         for (Statement* stmt : bb->Statements())
         {
             DISPSTMT(stmt);
-            replacer.StartStatement();
+            replacer.StartStatement(stmt);
             replacer.WalkTree(stmt->GetRootNodePointer(), nullptr);
 
             if (replacer.MadeChanges())
@@ -1790,6 +1819,15 @@ PhaseStatus Promotion::Run()
                 JITDUMP("New statement:\n");
                 DISPSTMT(stmt);
             }
+
+            if (replacer.MayHaveForwardSubOpportunity())
+            {
+                JITDUMP("Invoking forward sub due to a potential opportunity\n");
+                while ((stmt != bb->firstStmt()) && m_compiler->fgForwardSubStatement(stmt->GetPrevStmt()))
+                {
+                    m_compiler->fgRemoveStmt(bb, stmt->GetPrevStmt());
+                }
+            }
         }
 
         replacer.EndBlock();

src/coreclr/jit/promotion.h

@@ -248,10 +248,14 @@ class DecompositionPlan;
 
 class ReplaceVisitor : public GenTreeVisitor<ReplaceVisitor>
 {
+    friend class DecompositionPlan;
+
     jitstd::vector<AggregateInfo*>& m_aggregates;
     PromotionLiveness*              m_liveness;
     bool                            m_madeChanges         = false;
     bool                            m_hasPendingReadBacks = false;
+    bool                            m_mayHaveForwardSub   = false;
+    Statement*                      m_currentStmt         = nullptr;
     BasicBlock*                     m_currentBlock        = nullptr;
 
 public:
@@ -271,16 +275,23 @@ class ReplaceVisitor : public GenTreeVisitor<ReplaceVisitor>
         return m_madeChanges;
     }
 
+    bool MayHaveForwardSubOpportunity()
+    {
+        return m_mayHaveForwardSub;
+    }
+
     void StartBlock(BasicBlock* block)
     {
         m_currentBlock = block;
     }
 
     void EndBlock();
 
-    void StartStatement()
+    void StartStatement(Statement* stmt)
     {
-        m_madeChanges = false;
+        m_currentStmt       = stmt;
+        m_madeChanges       = false;
+        m_mayHaveForwardSub = false;
     }
 
     fgWalkResult PostOrderVisit(GenTree** use, GenTree* user);
@@ -290,6 +301,7 @@ class ReplaceVisitor : public GenTreeVisitor<ReplaceVisitor>
     void LoadStoreAroundCall(GenTreeCall* call, GenTree* user);
     bool IsPromotedStructLocalDying(GenTreeLclVarCommon* structLcl);
     void ReplaceLocal(GenTree** use, GenTree* user);
+    void CheckForwardSubForLastUse(unsigned lclNum);
     void StoreBeforeReturn(GenTreeUnOp* ret);
     void WriteBackBefore(GenTree** use, unsigned lcl, unsigned offs, unsigned size);
     bool MarkForReadBack(unsigned lcl, unsigned offs, unsigned size);

src/coreclr/jit/promotiondecomposition.cpp

@@ -50,6 +50,7 @@ class DecompositionPlan
     };
 
     Compiler*                       m_compiler;
+    ReplaceVisitor*                 m_replacer;
     jitstd::vector<AggregateInfo*>& m_aggregates;
     PromotionLiveness*              m_liveness;
     GenTree*                        m_store;
@@ -61,13 +62,15 @@ class DecompositionPlan
 
 public:
     DecompositionPlan(Compiler*                       comp,
+                      ReplaceVisitor*                 replacer,
                       jitstd::vector<AggregateInfo*>& aggregates,
                       PromotionLiveness*              liveness,
                       GenTree*                        store,
                       GenTree*                        src,
                       bool                            dstInvolvesReplacements,
                       bool                            srcInvolvesReplacements)
         : m_compiler(comp)
+        , m_replacer(replacer)
         , m_aggregates(aggregates)
         , m_liveness(liveness)
         , m_store(store)
@@ -718,6 +721,7 @@ class DecompositionPlan
                     if (srcDeaths.IsReplacementDying((unsigned)replacementIndex))
                     {
                         src->gtFlags |= GTF_VAR_DEATH;
+                        m_replacer->CheckForwardSubForLastUse(entry.FromLclNum);
                     }
                 }
             }
@@ -1083,7 +1087,7 @@ void ReplaceVisitor::HandleStore(GenTree** use, GenTree* user)
         DecompositionStatementList result;
         EliminateCommasInBlockOp(store, &result);
 
-        DecompositionPlan plan(m_compiler, m_aggregates, m_liveness, store, src, dstInvolvesReplacements,
+        DecompositionPlan plan(m_compiler, this, m_aggregates, m_liveness, store, src, dstInvolvesReplacements,
                                srcInvolvesReplacements);
 
         if (dstInvolvesReplacements)