Skip to content

JIT: Reorder physical promotion and forward sub #87265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 9, 2023
Merged

JIT: Reorder physical promotion and forward sub #87265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
151d847
JIT: Reorder physical promotion and forward sub
jakobbotsch Jun 8, 2023
33e3928
Fix "has previous statement" check
jakobbotsch Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
42 changes: 40 additions & 2 deletions src/coreclr/jit/promotion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
}
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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())
Expand All @@ -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();
Expand Down
16 changes: 14 additions & 2 deletions src/coreclr/jit/promotion.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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);
Expand All @@ -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);
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/jit/promotiondecomposition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class DecompositionPlan
};

Compiler* m_compiler;
ReplaceVisitor* m_replacer;
jitstd::vector<AggregateInfo*>& m_aggregates;
PromotionLiveness* m_liveness;
GenTree* m_store;
Expand All @@ -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)
Expand Down Expand Up @@ -731,6 +734,7 @@ class DecompositionPlan
if (srcDeaths.IsReplacementDying((unsigned)replacementIndex))
{
src->gtFlags |= GTF_VAR_DEATH;
m_replacer->CheckForwardSubForLastUse(entry.FromLclNum);
}
}
}
Expand Down Expand Up @@ -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)
Expand Down