Skip to content

JIT: one phi arg per pred #85546

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 1 commit into from
May 1, 2023
Merged

JIT: one phi arg per pred #85546

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
76 changes: 32 additions & 44 deletions src/coreclr/jit/ssabuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,8 @@ void SsaBuilder::InsertPhi(BasicBlock* block, unsigned lclNum)
}

//------------------------------------------------------------------------
// AddPhiArg: Add a new GT_PHI_ARG node to an existing GT_PHI node.
// AddPhiArg: Ensure an existing GT_PHI node contains an appropriate PhiArg
// for an ssa def arriving via pred
//
// Arguments:
// block - The block that contains the statement
Expand All @@ -563,14 +564,32 @@ void SsaBuilder::InsertPhi(BasicBlock* block, unsigned lclNum)
void SsaBuilder::AddPhiArg(
BasicBlock* block, Statement* stmt, GenTreePhi* phi, unsigned lclNum, unsigned ssaNum, BasicBlock* pred)
{
#ifdef DEBUG
// Make sure it isn't already present: we should only add each definition once.
// If there's already a phi arg for this pred, it had better have
// matching ssaNum, unless this block is a handler entry.
//
const bool isHandlerEntry = m_pCompiler->bbIsHandlerBeg(block);

for (GenTreePhi::Use& use : phi->Uses())
{
assert(use.GetNode()->AsPhiArg()->GetSsaNum() != ssaNum);
GenTreePhiArg* const phiArg = use.GetNode()->AsPhiArg();

if (phiArg->gtPredBB == pred)
{
if (phiArg->GetSsaNum() == ssaNum)
{
// We already have this (pred, ssaNum) phiArg
return;
Copy link
Member

Choose a reason for hiding this comment

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

Does this situation happen in practice? Seems odd we'd add the same phi arg twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was roughly copying what the previous code did. I'll find out if we ever hit this case.

Copy link
Member Author

@AndyAyersMS AndyAyersMS May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this can happen.

AddPhiArgsToSuccessors will enumerate all the successors of a block via GetAllSuccs, and for those successors that are try entries, will also add phi args to handlers reachable from the try (since we assume that execution of that try might throw an exception while the def from block is the current def).

But the GetAllSuccs enumerator has similar logic for enumerating handlers as successors of a block with a try successor, so we end up redundantly trying to add PhiArgs.

Seems like this is fixable, but best done as its own zero diff change.

}

// Add another ssaNum for this pred?
// Should only be possible at handler entries.
//
noway_assert(isHandlerEntry);
}
}
#endif // DEBUG

// Didn't find a match, add a new phi arg
//
var_types type = m_pCompiler->lvaGetDesc(lclNum)->TypeGet();

GenTree* phiArg = new (m_pCompiler, GT_PHI_ARG) GenTreePhiArg(type, lclNum, ssaNum, pred);
Expand Down Expand Up @@ -1186,29 +1205,12 @@ void SsaBuilder::AddPhiArgsToSuccessors(BasicBlock* block)
break;
}

GenTree* tree = stmt->GetRootNode();
GenTreePhi* phi = tree->gtGetOp2()->AsPhi();

unsigned lclNum = tree->AsOp()->gtOp1->AsLclVar()->GetLclNum();
unsigned ssaNum = m_renameStack.Top(lclNum);
// Search the arglist for an existing definition for ssaNum.
// (Can we assert that its the head of the list? This should only happen when we add
// during renaming for a definition that occurs within a try, and then that's the last
// value of the var within that basic block.)
GenTree* tree = stmt->GetRootNode();
GenTreePhi* phi = tree->gtGetOp2()->AsPhi();
unsigned lclNum = tree->AsOp()->gtOp1->AsLclVar()->GetLclNum();
unsigned ssaNum = m_renameStack.Top(lclNum);

bool found = false;
for (GenTreePhi::Use& use : phi->Uses())
{
if (use.GetNode()->AsPhiArg()->GetSsaNum() == ssaNum)
{
found = true;
break;
}
}
if (!found)
{
AddPhiArg(succ, stmt, phi, lclNum, ssaNum, block);
}
AddPhiArg(succ, stmt, phi, lclNum, ssaNum, block);
}

// Now handle memory.
Expand Down Expand Up @@ -1333,24 +1335,10 @@ void SsaBuilder::AddPhiArgsToSuccessors(BasicBlock* block)
continue;
}

GenTreePhi* phi = tree->gtGetOp2()->AsPhi();

unsigned ssaNum = m_renameStack.Top(lclNum);
GenTreePhi* phi = tree->gtGetOp2()->AsPhi();
unsigned ssaNum = m_renameStack.Top(lclNum);

// See if this ssaNum is already an arg to the phi.
bool alreadyArg = false;
for (GenTreePhi::Use& use : phi->Uses())
{
if (use.GetNode()->AsPhiArg()->GetSsaNum() == ssaNum)
{
alreadyArg = true;
break;
}
}
if (!alreadyArg)
{
AddPhiArg(handlerStart, stmt, phi, lclNum, ssaNum, block);
}
AddPhiArg(handlerStart, stmt, phi, lclNum, ssaNum, block);
}

// Now handle memory.
Expand Down