From 8f6af6e3d30df5858a09e6fc405565d63ff6a96a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 9 Jun 2023 22:53:22 +0200 Subject: [PATCH] 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. --- src/coreclr/jit/compiler.cpp | 8 ++--- src/coreclr/jit/promotion.cpp | 42 ++++++++++++++++++++-- src/coreclr/jit/promotion.h | 16 +++++++-- src/coreclr/jit/promotiondecomposition.cpp | 6 +++- 4 files changed, 63 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index df7e672d955..a25a04787c5 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/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; diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index f2f32daa9d7..caacd8c99c2 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/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(); diff --git a/src/coreclr/jit/promotion.h b/src/coreclr/jit/promotion.h index 24ebffe722e..c0b65bef76a 100644 --- a/src/coreclr/jit/promotion.h +++ b/src/coreclr/jit/promotion.h @@ -248,10 +248,14 @@ class DecompositionPlan; class ReplaceVisitor : public GenTreeVisitor { + friend class DecompositionPlan; + jitstd::vector& 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,6 +275,11 @@ public: return m_madeChanges; } + bool MayHaveForwardSubOpportunity() + { + return m_mayHaveForwardSub; + } + void StartBlock(BasicBlock* block) { m_currentBlock = block; @@ -278,9 +287,11 @@ public: 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 @@ private: 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); diff --git a/src/coreclr/jit/promotiondecomposition.cpp b/src/coreclr/jit/promotiondecomposition.cpp index 67f02dabf45..9be8ed9c8b0 100644 --- a/src/coreclr/jit/promotiondecomposition.cpp +++ b/src/coreclr/jit/promotiondecomposition.cpp @@ -50,6 +50,7 @@ class DecompositionPlan }; Compiler* m_compiler; + ReplaceVisitor* m_replacer; jitstd::vector& m_aggregates; PromotionLiveness* m_liveness; GenTree* m_store; @@ -61,6 +62,7 @@ class DecompositionPlan public: DecompositionPlan(Compiler* comp, + ReplaceVisitor* replacer, jitstd::vector& aggregates, PromotionLiveness* liveness, GenTree* store, @@ -68,6 +70,7 @@ public: bool dstInvolvesReplacements, bool srcInvolvesReplacements) : m_compiler(comp) + , m_replacer(replacer) , m_aggregates(aggregates) , m_liveness(liveness) , m_store(store) @@ -718,6 +721,7 @@ private: 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)