Skip to content
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

Copy prop equivalent phis #104458

Closed
wants to merge 5 commits into from
Closed

Copy prop equivalent phis #104458

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c1d159a
proof of concept
AndyAyersMS Jul 3, 2024
d29cead
fatal flaw -- no copy prop during global AP
AndyAyersMS Jul 4, 2024
e6ecd60
move to const prop
AndyAyersMS Jul 5, 2024
dd66fff
rework to improve TP
AndyAyersMS Jul 8, 2024
f510fae
revert AP changes, cleanup
AndyAyersMS Jul 8, 2024
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
171 changes: 159 additions & 12 deletions src/coreclr/jit/copyprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,23 @@ bool Compiler::optCopyProp(
assert((tree->gtFlags & GTF_VAR_DEF) == 0);
assert(tree->GetLclNum() == lclNum);

bool madeChanges = false;
LclVarDsc* varDsc = lvaGetDesc(lclNum);
ValueNum lclDefVN = varDsc->GetPerSsaData(tree->GetSsaNum())->m_vnPair.GetConservative();
bool madeChanges = false;
LclVarDsc* const varDsc = lvaGetDesc(lclNum);
LclSsaVarDsc* const varSsaDsc = varDsc->GetPerSsaData(tree->GetSsaNum());
GenTree* const varDefTree = varSsaDsc->GetDefNode();
BasicBlock* const varDefBlock = varSsaDsc->GetBlock();
ValueNum lclDefVN = varSsaDsc->m_vnPair.GetConservative();
assert(lclDefVN != ValueNumStore::NoVN);

bool const varDefTreeIsPhiDef = (varDefTree != nullptr) && varDefTree->IsPhiDefn();
bool varDefTreeIsPhiDefAtCycleEntry = false;

if (varDefTreeIsPhiDef)
{
FlowGraphNaturalLoop* const loop = m_blockToLoop->GetLoop(varDefBlock);
varDefTreeIsPhiDefAtCycleEntry = (loop != nullptr) && (loop->GetHeader() == varDefBlock);
}

for (LclNumToLiveDefsMap::Node* const iter : LclNumToLiveDefsMap::KeyValueIteration(curSsaName))
{
unsigned newLclNum = iter->GetKey();
Expand All @@ -190,7 +202,129 @@ bool Compiler::optCopyProp(

if (newLclDefVN != lclDefVN)
{
continue;
// If the definition is a phi def, and is defined in a block that has a backedge,
// we may be able to prove equivalence with some other phi def in that block, despite
// the fact that the VNs do not match.
//
// This happens because VN is one-pass and doesn't know the VNs for backedge PhiArgs,
// so the VNs for phi defs for loop entry blocks is always a novel opaque VN. But
// we can query those backedge VNs after VN is done, and if all Phi Arg VNs match, then
// the two Phi Defs are equivalent and could have had the same VN.
Comment on lines +209 to +212
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious what some of these cases look like... the fact that we have a phi in the header should mean we saw a def inside the loop, so in this case that def has to be redefining the local to have the same value as when we come from outside the loop. I would (naively, maybe?) expect that to be rare.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's defining a local to have the same value as some other local at the start of a loop. So yes both have the same value on initial entry and end up with the same value along the backedge. #95645 (comment) has some notes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely the fallout from of pre-increment or similar, the code modifies a value and then uses the pre-modified value... eg

runtime/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.Enumerator.cs

Lines 62 to 65 in 21cde69

public bool MoveNext()
{
return ++_index < _array.Length;
}

This requires a temp in our IR. Morph and loop inversion conspire to make both old and new value live around the loop.

//
bool foundEquivalentPhi = false;

// Is the new local def from the same block?
//
if (varDefTreeIsPhiDefAtCycleEntry && (varDefBlock == newLclSsaDef->GetBlock()))
{
GenTreeLclVarCommon* const otherTree = newLclSsaDef->GetDefNode();

// Is it also a phi definition? Do the types match?
//
if ((otherTree != nullptr) && otherTree->IsPhiDefn() && (otherTree->TypeGet() == tree->TypeGet()))
{
// Are all the Phi Args VN equivalent?
//
GenTreePhi* const treePhi = varDefTree->AsLclVar()->Data()->AsPhi();
GenTreePhi* const otherPhi = otherTree->AsLclVar()->Data()->AsPhi();
GenTreePhi::UseIterator treeIter = treePhi->Uses().begin();
GenTreePhi::UseIterator treeEnd = treePhi->Uses().end();
GenTreePhi::UseIterator otherIter = otherPhi->Uses().begin();
GenTreePhi::UseIterator otherEnd = otherPhi->Uses().end();

bool phiArgsAreEquivalent = true;

for (; (treeIter != treeEnd) && (otherIter != otherEnd); ++treeIter, ++otherIter)
{
GenTreePhiArg* const treePhiArg = treeIter->GetNode()->AsPhiArg();
GenTreePhiArg* const otherPhiArg = otherIter->GetNode()->AsPhiArg();

assert(treePhiArg->gtPredBB == otherPhiArg->gtPredBB);

ValueNum treePhiArgVN = treePhiArg->gtVNPair.GetConservative();
ValueNum otherPhiArgVN = otherPhiArg->gtVNPair.GetConservative();

const bool phiArgIsFromDfsBackedge = m_dfsTree->IsAncestor(varDefBlock, treePhiArg->gtPredBB);

if ((treePhiArgVN == ValueNumStore::NoVN) && phiArgIsFromDfsBackedge)
{
// Consult SSA defs
//
LclSsaVarDsc* const treePhiArgSsaDef =
lvaGetDesc(treePhiArg)->GetPerSsaData(treePhiArg->GetSsaNum());
ValueNum treePhiArgSsaDefVN = treePhiArgSsaDef->m_vnPair.GetConservative();

// Opportunisitcally update the PhiArgVNs, both to short-circuit future checks here
// in copy prop, and help later phases that also scan phi arg lists (eg RBO).
// (consider doing this as a VN post pass?)
//
if (treePhiArgSsaDefVN != treePhiArgVN)
{
JITDUMP("Updating backedge phi arg [%06u] vn to " FMT_VN "\n", dspTreeID(treePhiArg),
treePhiArgSsaDefVN);
treePhiArg->SetVNs(treePhiArgSsaDef->m_vnPair);
}

treePhiArgVN = treePhiArgSsaDefVN;
}

if ((otherPhiArgVN == ValueNumStore::NoVN) && phiArgIsFromDfsBackedge)
{
LclSsaVarDsc* const otherPhiArgSsaDef =
lvaGetDesc(otherPhiArg)->GetPerSsaData(otherPhiArg->GetSsaNum());
ValueNum otherPhiArgSsaDefVN = otherPhiArgSsaDef->m_vnPair.GetConservative();

// Opportunisitcally update the PhiArgVNs, both to short-circuit future checks here
// in copy prop, and help later phases that also scan phi arg lists (eg RBO).
// (consider doing this as a VN post pass?)
//
if (otherPhiArgSsaDefVN != otherPhiArgVN)
{
JITDUMP("Updating backedge phi arg [%06u] vn to " FMT_VN "\n", dspTreeID(otherPhiArg),
otherPhiArgSsaDefVN);
otherPhiArg->SetVNs(otherPhiArgSsaDef->m_vnPair);
}

otherPhiArgVN = otherPhiArgSsaDefVN;
}

// If the updated VNs differ, the phis are not equivalent
//
if (treePhiArgVN != otherPhiArgVN)
{
phiArgsAreEquivalent = false;
break;
}

// If we failed to find meaningful VNs, the phis are not equivalent
//
if (treePhiArgVN == ValueNumStore::NoVN)
{
phiArgsAreEquivalent = false;
break;
}
}

// If we didn't verify all phi args we have failed to prove equivalence
//
phiArgsAreEquivalent &= (treeIter == treeEnd);
phiArgsAreEquivalent &= (otherIter == otherEnd);

if (phiArgsAreEquivalent)
{
JITDUMP("Found equivalent phi defs: [%06u] and [%06u]\n", dspTreeID(treePhi),
dspTreeID(otherPhi));
foundEquivalentPhi = true;
}
}
}

// VNs differed, and we didn't find phis equivalent, move on to the next candidate.
//
if (!foundEquivalentPhi)
{
continue;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nesting here is pretty deep... consider introducing a function with some early outs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was tempted to try that to see if helped TP any, but haven't yet.

}

// It may not be profitable to propagate a 'doNotEnregister' lclVar to an existing use of an
Expand Down Expand Up @@ -257,8 +391,27 @@ bool Compiler::optCopyProp(
unsigned newSsaNum = newLclVarDsc->GetSsaNumForSsaDef(newLclSsaDef);
assert(newSsaNum != SsaConfig::RESERVED_SSA_NUM);

tree->AsLclVarCommon()->SetLclNum(newLclNum);
tree->AsLclVarCommon()->SetSsaNum(newSsaNum);
tree->SetLclNum(newLclNum);
tree->SetSsaNum(newSsaNum);

// Update VN to match, and propagate up through any enclosing commas.
// (we could in principle try updating through other parents, but
// we lack VN's context for memory, so can't get them all).
//
if (newLclDefVN != lclDefVN)
{
tree->SetVNs(newLclSsaDef->m_vnPair);
GenTree* parent = tree->gtGetParent(nullptr);

while ((parent != nullptr) && parent->OperIs(GT_COMMA))
{
JITDUMP(" Updating COMMA parent VN [%06u]\n", dspTreeID(parent));
ValueNumPair op1Xvnp = vnStore->VNPExceptionSet(parent->AsOp()->gtOp1->gtVNPair);
parent->SetVNs(vnStore->VNPWithExc(parent->AsOp()->gtOp2->gtVNPair, op1Xvnp));
parent = tree->gtGetParent(nullptr);
}
}

gtUpdateSideEffects(stmt, tree);
newLclSsaDef->AddUse(block);

Expand Down Expand Up @@ -334,12 +487,6 @@ void Compiler::optCopyPropPushDef(GenTree* defNode, GenTreeLclVarCommon* lclNode
else if (lclNode->HasSsaName())
{
unsigned ssaNum = lclNode->GetSsaNum();
if ((defNode != nullptr) && defNode->IsPhiDefn())
{
// TODO-CQ: design better heuristics for propagation and remove this.
ssaNum = SsaConfig::RESERVED_SSA_NUM;
}

pushDef(lclNum, ssaNum);
}
}
Expand Down
Loading