Skip to content

Commit

Permalink
Handle embedded assignments in copy propagation (#63447)
Browse files Browse the repository at this point in the history 
* Clean things up a little

Delete redundant conditions, use "LclVarDsc*", rename locals for clarity.

* Delete a redundant condition

For actual def nodes, GTF_VAR_CAST will never be set, it is
only set in "optNarrowTree" for uses.

For "def nodes" that are actually uses (parameters), the VNs
will never match anyway.

* Handle embedded assignments in copy propagation

Previously, as the comments in copy propagation tell us, it
did not handle "intervening", or not-top-level definitions of
locals, instead opting to maintain a dedicated kill set of them.

This is obviously a CQ problem, but also a TP one, as it meant
there had to be a second pass over the statement's IR, where
the definitions would be pushed on the stack.

This change does away with that, instead pushing new definitions
as they are encountered in execution order, and simultaneously
propagating on uses. Notably, this means the code now needs to
look at the real definition nodes, i. e. ASGs, not the LHS locals,
as those are encountered first in canonical execution order, i. e.
for a tree like:

```
  ASG
    LCL_VAR V00 "def"
    ADD
      LCL_VAR V00
      LCL_VAR V00
```

Were we to use the "def" as the definition point, we would wrongly
push it as the definition on the stack, even as the assignments
itself hasn't happened yet at that point.

There are nice diffs with this change, all resulting from unblocked
propagations, and mostly coming from setup arguments under calls.

* Simplify optIsSsaLocal
  • Loading branch information
@SingleAccretion
SingleAccretion committed Jan 21, 2022
1 parent 1d3c23d commit 651fa6d
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 148 deletions.
7 changes: 2 additions & 5 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7384,14 +7384,11 @@ class Compiler
typedef ArrayStack<GenTree*> GenTreePtrStack;
typedef JitHashTable<unsigned, JitSmallPrimitiveKeyFuncs<unsigned>, GenTreePtrStack*> LclNumToGenTreePtrStack;

// Kill set to track variables with intervening definitions.
VARSET_TP optCopyPropKillSet;

// Copy propagation functions.
void optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, LclNumToGenTreePtrStack* curSsaName);
void optCopyProp(Statement* stmt, GenTreeLclVarCommon* tree, unsigned lclNum, LclNumToGenTreePtrStack* curSsaName);
void optBlockCopyPropPopStacks(BasicBlock* block, LclNumToGenTreePtrStack* curSsaName);
void optBlockCopyProp(BasicBlock* block, LclNumToGenTreePtrStack* curSsaName);
unsigned optIsSsaLocal(GenTree* tree);
unsigned optIsSsaLocal(GenTreeLclVarCommon* lclNode);
int optCopyProp_LclVarScore(LclVarDsc* lclVarDsc, LclVarDsc* copyVarDsc, bool preferOp2);
void optVnCopyProp();
INDEBUG(void optDumpCopyPropStack(LclNumToGenTreePtrStack* curSsaName));
Expand Down
234 changes: 91 additions & 143 deletions src/coreclr/jit/copyprop.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,16 @@ void Compiler::optBlockCopyPropPopStacks(BasicBlock* block, LclNumToGenTreePtrSt
{
for (GenTree* const tree : stmt->TreeList())
{
if (!tree->IsLocal())
{
continue;
}
const unsigned lclNum = optIsSsaLocal(tree);
if (lclNum == BAD_VAR_NUM)
{
continue;
}
if (tree->gtFlags & GTF_VAR_DEF)
GenTreeLclVarCommon* lclDefNode = nullptr;
if (tree->OperIs(GT_ASG) && tree->DefinesLocal(this, &lclDefNode))
{
const unsigned lclNum = optIsSsaLocal(lclDefNode);

if (lclNum == BAD_VAR_NUM)
{
continue;
}

GenTreePtrStack* stack = nullptr;
curSsaName->Lookup(lclNum, &stack);
stack->Pop();
Expand Down Expand Up @@ -123,97 +122,67 @@ int Compiler::optCopyProp_LclVarScore(LclVarDsc* lclVarDsc, LclVarDsc* copyVarDs
// definitions share the same value number. If so, then we can make the replacement.
//
// Arguments:
// block - Block the tree belongs to
// stmt - Statement the tree belongs to
// tree - The tree to perform copy propagation on
// tree - The local tree to perform copy propagation on
// lclNum - The local number of said tree
// curSsaName - The map from lclNum to its recently live definitions as a stack

void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, LclNumToGenTreePtrStack* curSsaName)
void Compiler::optCopyProp(Statement* stmt,
GenTreeLclVarCommon* tree,
unsigned lclNum,
LclNumToGenTreePtrStack* curSsaName)
{
// TODO-Review: EH successor/predecessor iteration seems broken.
if (block->bbCatchTyp == BBCT_FINALLY || block->bbCatchTyp == BBCT_FAULT)
{
return;
}

// If not local nothing to do.
if (!tree->IsLocal())
{
return;
}
if (tree->OperGet() == GT_PHI_ARG || tree->OperGet() == GT_LCL_FLD)
{
return;
}

// Propagate only on uses.
if (tree->gtFlags & GTF_VAR_DEF)
{
return;
}
const unsigned lclNum = optIsSsaLocal(tree);

// Skip non-SSA variables.
if (lclNum == BAD_VAR_NUM)
{
return;
}

assert((lclNum != BAD_VAR_NUM) && (optIsSsaLocal(tree) == lclNum) && ((tree->gtFlags & GTF_VAR_DEF) == 0));
assert(tree->gtVNPair.GetConservative() != ValueNumStore::NoVN);

for (LclNumToGenTreePtrStack::KeyIterator iter = curSsaName->Begin(); !iter.Equal(curSsaName->End()); ++iter)
{
unsigned newLclNum = iter.Get();

GenTree* op = iter.GetValue()->Top();

// Nothing to do if same.
if (lclNum == newLclNum)
{
continue;
}

// Skip variables with assignments embedded in the statement (i.e., with a comma). Because we
// are not currently updating their SSA names as live in the copy-prop pass of the stmt.
if (VarSetOps::IsMember(this, optCopyPropKillSet, lvaTable[newLclNum].lvVarIndex))
{
continue;
}
LclVarDsc* varDsc = lvaGetDesc(lclNum);
LclVarDsc* newLclVarDsc = lvaGetDesc(newLclNum);
GenTree* newLclDefNode = iter.GetValue()->Top(); // Note that this "def" node can actually be a use (for
// parameters and other use-before-def locals).

// Do not copy propagate if the old and new lclVar have different 'doNotEnregister' settings.
// This is primarily to avoid copy propagating to IND(ADDR(LCL_VAR)) where the replacement lclVar
// is not marked 'lvDoNotEnregister'.
// However, in addition, it may not be profitable to propagate a 'doNotEnregister' lclVar to an
// existing use of an enregisterable lclVar.

if (lvaTable[lclNum].lvDoNotEnregister != lvaTable[newLclNum].lvDoNotEnregister)
if (varDsc->lvDoNotEnregister != newLclVarDsc->lvDoNotEnregister)
{
continue;
}

if (op->gtFlags & GTF_VAR_CAST)
if ((gsShadowVarInfo != nullptr) && newLclVarDsc->lvIsParam &&
(gsShadowVarInfo[newLclNum].shadowCopy == lclNum))
{
continue;
}
if (gsShadowVarInfo != nullptr && lvaTable[newLclNum].lvIsParam &&
gsShadowVarInfo[newLclNum].shadowCopy == lclNum)
{
continue;
}
ValueNum opVN = GetUseAsgDefVNOrTreeVN(op);
if (opVN == ValueNumStore::NoVN)

ValueNum newLclDefVN = GetUseAsgDefVNOrTreeVN(newLclDefNode);
if (newLclDefVN == ValueNumStore::NoVN)
{
continue;
}
if (op->TypeGet() != tree->TypeGet())

if (newLclDefNode->TypeGet() != tree->TypeGet())
{
continue;
}
if (opVN != tree->gtVNPair.GetConservative())

if (newLclDefVN != tree->gtVNPair.GetConservative())
{
continue;
}
if (optCopyProp_LclVarScore(lvaGetDesc(lclNum), lvaGetDesc(newLclNum), true) <= 0)

if (optCopyProp_LclVarScore(varDsc, newLclVarDsc, true) <= 0)
{
continue;
}
Expand All @@ -230,34 +199,25 @@ void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, Lc
// node x2 = phi(x0, x1) which can then be used to substitute 'c' with. But because of pruning
// there would be no such phi node. To solve this we'll check if 'x' is live, before replacing
// 'c' with 'x.'
if (!lvaTable[newLclNum].lvVerTypeInfo.IsThisPtr())
{
if (lvaTable[newLclNum].IsAddressExposed())
{
continue;
}

// We compute liveness only on tracked variables. So skip untracked locals.
if (!lvaTable[newLclNum].lvTracked)
{
continue;
}
// We compute liveness only on tracked variables. And all SSA locals are tracked.
assert(lvaGetDesc(newLclNum)->lvTracked);

// Because of this dependence on live variable analysis, CopyProp phase is immediately
// after Liveness, SSA and VN.
if (!VarSetOps::IsMember(this, compCurLife, lvaTable[newLclNum].lvVarIndex))
{
continue;
}
// Because of this dependence on live variable analysis, CopyProp phase is immediately
// after Liveness, SSA and VN.
if ((newLclNum != info.compThisArg) && !VarSetOps::IsMember(this, compCurLife, newLclVarDsc->lvVarIndex))
{
continue;
}

unsigned newSsaNum = SsaConfig::RESERVED_SSA_NUM;
if (op->gtFlags & GTF_VAR_DEF)
if (newLclDefNode->gtFlags & GTF_VAR_DEF)
{
newSsaNum = GetSsaNumForLocalVarDef(op);
newSsaNum = GetSsaNumForLocalVarDef(newLclDefNode);
}
else // parameters, this pointer etc.
{
newSsaNum = op->AsLclVarCommon()->GetSsaNum();
newSsaNum = newLclDefNode->AsLclVarCommon()->GetSsaNum();
}

if (newSsaNum == SsaConfig::RESERVED_SSA_NUM)
Expand All @@ -271,48 +231,42 @@ void Compiler::optCopyProp(BasicBlock* block, Statement* stmt, GenTree* tree, Lc
JITDUMP("VN based copy assertion for ");
printTreeID(tree);
printf(" V%02d " FMT_VN " by ", lclNum, tree->GetVN(VNK_Conservative));
printTreeID(op);
printf(" V%02d " FMT_VN ".\n", newLclNum, op->GetVN(VNK_Conservative));
gtDispTree(tree, nullptr, nullptr, true);
printTreeID(newLclDefNode);
printf(" V%02d " FMT_VN ".\n", newLclNum, newLclDefNode->GetVN(VNK_Conservative));
DISPNODE(tree);
}
#endif

tree->AsLclVarCommon()->SetLclNum(newLclNum);
tree->AsLclVarCommon()->SetSsaNum(newSsaNum);
gtUpdateSideEffects(stmt, tree);

#ifdef DEBUG
if (verbose)
{
printf("copy propagated to:\n");
gtDispTree(tree, nullptr, nullptr, true);
DISPNODE(tree);
}
#endif
break;
}
return;
}

//------------------------------------------------------------------------------
// optIsSsaLocal : helper to check if the tree is a local that participates in SSA numbering.
//
// Arguments:
// tree - The tree to perform the check on;
// lclNode - The local tree to perform the check on;
//
// Returns:
// - lclNum if the local is participating in SSA;
// - fieldLclNum if the parent local can be replaced by its only field;
// - BAD_VAR_NUM otherwise.
//
unsigned Compiler::optIsSsaLocal(GenTree* tree)
unsigned Compiler::optIsSsaLocal(GenTreeLclVarCommon* lclNode)
{
if (!tree->IsLocal())
{
return BAD_VAR_NUM;
}

GenTreeLclVarCommon* lclNode = tree->AsLclVarCommon();
unsigned lclNum = lclNode->GetLclNum();
LclVarDsc* varDsc = lvaGetDesc(lclNum);
unsigned lclNum = lclNode->GetLclNum();
LclVarDsc* varDsc = lvaGetDesc(lclNum);

if (!lvaInSsa(lclNum) && varDsc->CanBeReplacedWithItsField(this))
{
Expand Down Expand Up @@ -355,68 +309,63 @@ void Compiler::optBlockCopyProp(BasicBlock* block, LclNumToGenTreePtrStack* curS
VarSetOps::Assign(this, compCurLife, block->bbLiveIn);
for (Statement* const stmt : block->Statements())
{
VarSetOps::ClearD(this, optCopyPropKillSet);

// Walk the tree to find if any local variable can be replaced with current live definitions.
// Simultaneously, push live definitions on the stack - that logic must be in sync with the
// SSA renaming process.
for (GenTree* const tree : stmt->TreeList())
{
treeLifeUpdater.UpdateLife(tree);

optCopyProp(block, stmt, tree, curSsaName);

// TODO-Review: Merge this loop with the following loop to correctly update the
// live SSA num while also propagating copies.
//
// 1. This loop performs copy prop with currently live (on-top-of-stack) SSA num.
// 2. The subsequent loop maintains a stack for each lclNum with
// currently active SSA numbers when definitions are encountered.
//
// If there is an embedded definition using a "comma" in a stmt, then the currently
// live SSA number will get updated only in the next loop (2). However, this new
// definition is now supposed to be live (on tos). If we did not update the stacks
// using (2), copy prop (1) will use a SSA num defined outside the stmt ignoring the
// embedded update. Killing the variable is a simplification to produce 0 ASM diffs
// for an update release.
//
const unsigned lclNum = optIsSsaLocal(tree);
if ((lclNum != BAD_VAR_NUM) && (tree->gtFlags & GTF_VAR_DEF))
GenTreeLclVarCommon* lclDefNode = nullptr;
if (tree->OperIs(GT_ASG) && tree->DefinesLocal(this, &lclDefNode))
{
VarSetOps::AddElemD(this, optCopyPropKillSet, lvaTable[lclNum].lvVarIndex);
}
}
const unsigned lclNum = optIsSsaLocal(lclDefNode);

// This logic must be in sync with SSA renaming process.
for (GenTree* const tree : stmt->TreeList())
{
const unsigned lclNum = optIsSsaLocal(tree);
if (lclNum == BAD_VAR_NUM)
{
continue;
}
if (lclNum == BAD_VAR_NUM)
{
continue;
}

// As we encounter a definition add it to the stack as a live definition.
if (tree->gtFlags & GTF_VAR_DEF)
{
GenTreePtrStack* stack;
if (!curSsaName->Lookup(lclNum, &stack))
{
stack = new (curSsaName->GetAllocator()) GenTreePtrStack(curSsaName->GetAllocator());
}
stack->Push(tree);
stack->Push(lclDefNode);
curSsaName->Set(lclNum, stack, LclNumToGenTreePtrStack::Overwrite);
}
// If we encounter first use of a param or this pointer add it as a live definition.
// Since they are always live, do it only once.
else if ((tree->gtOper == GT_LCL_VAR) && !(tree->gtFlags & GTF_VAR_USEASG) &&
(lvaTable[lclNum].lvIsParam || lvaTable[lclNum].lvVerTypeInfo.IsThisPtr()))
// TODO-CQ: propagate on LCL_FLDs too.
else if (tree->OperIs(GT_LCL_VAR) && ((tree->gtFlags & GTF_VAR_DEF) == 0))
{
GenTreePtrStack* stack;
if (!curSsaName->Lookup(lclNum, &stack))
const unsigned lclNum = optIsSsaLocal(tree->AsLclVarCommon());

if (lclNum == BAD_VAR_NUM)
{
stack = new (curSsaName->GetAllocator()) GenTreePtrStack(curSsaName->GetAllocator());
stack->Push(tree);
curSsaName->Set(lclNum, stack);
continue;
}

// If we encounter first use of a param or this pointer add it as a live definition.
// Since they are always live, we'll do it only once.
if (lvaGetDesc(lclNum)->lvIsParam || (lclNum == info.compThisArg))
{
GenTreePtrStack* stack;
if (!curSsaName->Lookup(lclNum, &stack))
{
assert(tree->AsLclVarCommon()->GetSsaNum() == SsaConfig::FIRST_SSA_NUM);

stack = new (curSsaName->GetAllocator()) GenTreePtrStack(curSsaName->GetAllocator());
stack->Push(tree);
curSsaName->Set(lclNum, stack);
}
}

// TODO-Review: EH successor/predecessor iteration seems broken.
if ((block->bbCatchTyp == BBCT_FINALLY) || (block->bbCatchTyp == BBCT_FAULT))
{
continue;
}

optCopyProp(stmt, tree->AsLclVarCommon(), lclNum, curSsaName);
}
}
}
Expand Down Expand Up @@ -462,7 +411,6 @@ void Compiler::optVnCopyProp()
}

VarSetOps::AssignNoCopy(this, compCurLife, VarSetOps::MakeEmpty(this));
VarSetOps::AssignNoCopy(this, optCopyPropKillSet, VarSetOps::MakeEmpty(this));

class CopyPropDomTreeVisitor : public DomTreeVisitor<CopyPropDomTreeVisitor>
{
Expand Down

0 comments on commit 651fa6d

Please sign in to comment.