From 151d8474e4f135d7400ec2396a10ad8efb98d02e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 8 Jun 2023 12:18:20 +0200 Subject: [PATCH 1/2] JIT: Reorder physical promotion and forward sub 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 | 41 ++++++++++++++++++++-- src/coreclr/jit/promotion.h | 16 +++++++-- src/coreclr/jit/promotiondecomposition.cpp | 6 +++- 4 files changed, 62 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index df7e672d955ea1..a25a04787c5fa6 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 f2f32daa9d76c8..00bcc868843f87 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,29 @@ 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) +{ + Statement* prevStmt = m_currentStmt->GetPrevStmt(); + if (prevStmt == nullptr) + { + return; + } + + GenTree* node = prevStmt->GetRootNode(); + if (node->OperIsLocalStore() && (node->AsLclVarCommon()->GetLclNum() == lclNum)) + { + m_mayHaveForwardSub = true; + } +} + //------------------------------------------------------------------------ // StoreBeforeReturn: // Handle a return of a potential struct local. @@ -1780,7 +1808,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 +1818,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 24ebffe722e7ea..c0b65bef76a3ef 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 @@ class ReplaceVisitor : public GenTreeVisitor return m_madeChanges; } + bool MayHaveForwardSubOpportunity() + { + return m_mayHaveForwardSub; + } + void StartBlock(BasicBlock* block) { m_currentBlock = block; @@ -278,9 +287,11 @@ class ReplaceVisitor : public GenTreeVisitor 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 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 d0df8b9e17fa62..af70d8145956cb 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 @@ class DecompositionPlan bool dstInvolvesReplacements, bool srcInvolvesReplacements) : m_compiler(comp) + , m_replacer(replacer) , m_aggregates(aggregates) , m_liveness(liveness) , m_store(store) @@ -731,6 +734,7 @@ class DecompositionPlan if (srcDeaths.IsReplacementDying((unsigned)replacementIndex)) { src->gtFlags |= GTF_VAR_DEATH; + m_replacer->CheckForwardSubForLastUse(entry.FromLclNum); } } } @@ -1009,7 +1013,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) From 33e3928cdd3072c0eb9605ed7e23c4b7332b0171 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 8 Jun 2023 12:25:05 +0200 Subject: [PATCH 2/2] Fix "has previous statement" check --- src/coreclr/jit/promotion.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index 00bcc868843f87..caacd8c99c24a4 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -1555,14 +1555,15 @@ void ReplaceVisitor::ReplaceLocal(GenTree** use, GenTree* user) // void ReplaceVisitor::CheckForwardSubForLastUse(unsigned lclNum) { - Statement* prevStmt = m_currentStmt->GetPrevStmt(); - if (prevStmt == nullptr) + if (m_currentBlock->firstStmt() == m_currentStmt) { return; } - GenTree* node = prevStmt->GetRootNode(); - if (node->OperIsLocalStore() && (node->AsLclVarCommon()->GetLclNum() == lclNum)) + Statement* prevStmt = m_currentStmt->GetPrevStmt(); + GenTree* prevNode = prevStmt->GetRootNode(); + + if (prevNode->OperIsLocalStore() && (prevNode->AsLclVarCommon()->GetLclNum() == lclNum)) { m_mayHaveForwardSub = true; }