Skip to content

Commit

Permalink
Improvements to loop hoisting (#71504)
Browse files Browse the repository at this point in the history 
* Improvements to loop hoisting

- optHoistLoopNest(): (1) no need to check if a pre-header has already been added;
pre-headers are unique to a loop if they exist. (2) simplify the "append to list" code.
- optHoistThisLoop(): (1) allow adding multiple pre-headers after every "IDom" addition,
if there are any in the right order. (2) Simplify addition of remaining pre-headers.
- optLoopEntry(): technically, you shouldn't look at `bbJumpDest`
unless you know the branch kind is one that sets it. So change the condition.

Also, fix the output of why something wasn't hoisted. The concept of "hoistable" implies
"invariant" (but not the reverse). So print the least specific failure reason. These were
backwards before. For instance, if we couldn't hoist a top node, but it was invariant, we
would, oddly, print "not invariant" instead of "not hoistable". In the case where it isn't
hoistable, and also isn't invariant, it makes more sense to print "not invariant", which
also implies "not hoistable".

Add dumping the OpName for the Pre/PostOrderVisit output, to help avoid cross-referencing
the node number all the time.

No asm diffs.

* Feedback
  • Loading branch information
@BruceForstall
BruceForstall committed Jul 1, 2022
1 parent 9b10634 commit f28904a
Showing 1 changed file with 51 additions and 64 deletions.
115 changes: 51 additions & 64 deletions src/coreclr/jit/optimizer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3429,7 +3429,15 @@ BasicBlock* Compiler::optLoopEntry(BasicBlock* preHeader)
{
assert((preHeader->bbFlags & BBF_LOOP_PREHEADER) != 0);

return (preHeader->bbJumpDest == nullptr) ? preHeader->bbNext : preHeader->bbJumpDest;
if (preHeader->KindIs(BBJ_NONE))
{
return preHeader->bbNext;
}
else
{
assert(preHeader->KindIs(BBJ_ALWAYS));
return preHeader->bbJumpDest;
}
}

//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -6543,45 +6551,36 @@ bool Compiler::optHoistLoopNest(unsigned lnum, LoopHoistContext* hoistCtxt)
m_loopsConsidered++;
#endif // LOOP_HOIST_STATS

BasicBlockList* preHeadersOfChildLoops = nullptr;
BasicBlockList* firstPreHeader = nullptr;
BasicBlockList* firstPreHeader = nullptr;
BasicBlockList** preHeaderAppendPtr = &firstPreHeader;

bool modified = false;

if (optLoopTable[lnum].lpChild != BasicBlock::NOT_IN_LOOP)
{
BitVecTraits m_visitedTraits(fgBBNumMax * 2, this);
BitVec m_visited(BitVecOps::MakeEmpty(&m_visitedTraits));

for (unsigned child = optLoopTable[lnum].lpChild; child != BasicBlock::NOT_IN_LOOP;
child = optLoopTable[child].lpSibling)
{
modified |= optHoistLoopNest(child, hoistCtxt);

if (optLoopTable[child].lpFlags & LPFLG_HAS_PREHEAD)
{
// If any preheaders were found, add them to the tracking list
// If a pre-header is found, add it to the tracking list. Most likely, the recursive call
// to hoist from the `child` loop nest found something to hoist and created a pre-header
// where the hoisted code was placed.

BasicBlock* preHeaderBlock = optLoopTable[child].lpHead;
if (!BitVecOps::IsMember(&m_visitedTraits, m_visited, preHeaderBlock->bbNum))
{
BitVecOps::AddElemD(&m_visitedTraits, m_visited, preHeaderBlock->bbNum);
JITDUMP(" PREHEADER: " FMT_BB "\n", preHeaderBlock->bbNum);
JITDUMP(" PREHEADER: " FMT_BB "\n", preHeaderBlock->bbNum);

// Here, we are arranging the blocks in reverse execution order, so when they are pushed
// on the stack that hoist these blocks further sees them in execution order.
if (firstPreHeader == nullptr)
{
preHeadersOfChildLoops = new (this, CMK_LoopHoist) BasicBlockList(preHeaderBlock, nullptr);
firstPreHeader = preHeadersOfChildLoops;
}
else
{
preHeadersOfChildLoops->next =
new (this, CMK_LoopHoist) BasicBlockList(preHeaderBlock, nullptr);
preHeadersOfChildLoops = preHeadersOfChildLoops->next;
}
}
// Here, we are arranging the blocks in reverse lexical order, so when they are removed from the
// head of this list and pushed on the stack of hoisting blocks to process, they are in
// forward lexical order when popped. Note that for child loops `i` and `j`, in order `i`
// followed by `j` in the `lpSibling` list, that `j` is actually first in lexical order
// and that makes these blocks arranged in reverse lexical order in this list.
// That is, the sibling list is in reverse lexical order.
// Note: the sibling list order should not matter to any algorithm; this order is not guaranteed.
*preHeaderAppendPtr = new (this, CMK_LoopHoist) BasicBlockList(preHeaderBlock, nullptr);
preHeaderAppendPtr = &((*preHeaderAppendPtr)->next);
}
}
}
Expand Down Expand Up @@ -6718,8 +6717,7 @@ bool Compiler::optHoistThisLoop(unsigned lnum, LoopHoistContext* hoistCtxt, Basi
// and it is safe to evaluate the tree early.
//
ArrayStack<BasicBlock*> defExec(getAllocatorLoopHoist());

bool pushAllPreheaders = false;
BasicBlockList* preHeadersList = existingPreHeaders;

if (pLoopDsc->lpExitCnt == 1)
{
Expand All @@ -6735,49 +6733,39 @@ bool Compiler::optHoistThisLoop(unsigned lnum, LoopHoistContext* hoistCtxt, Basi
//
// TODO-CQ: In future, we should create preheaders upfront before building
// dominators so we don't have to do this extra work here.
BasicBlock* cur = pLoopDsc->lpExit;
BasicBlockList* preHeadersList = existingPreHeaders;

BasicBlock* cur = pLoopDsc->lpExit;
while (cur != nullptr && pLoopDsc->lpContains(cur) && cur != pLoopDsc->lpEntry)
{
JITDUMP(" -- " FMT_BB " (dominate exit block)\n", cur->bbNum);
defExec.Push(cur);
cur = cur->bbIDom;

if (preHeadersList != nullptr)
for (; preHeadersList != nullptr; preHeadersList = preHeadersList->next)
{
BasicBlock* preHeaderBlock = preHeadersList->block;
BasicBlock* lpEntry = optLoopEntry(preHeaderBlock);
if (cur->bbNum < lpEntry->bbNum)
{
JITDUMP(" -- " FMT_BB " (preheader of " FMT_LP ")\n", preHeaderBlock->bbNum,
lpEntry->bbNatLoopNum);

defExec.Push(preHeaderBlock);
preHeadersList = preHeadersList->next;
}
else
{
break;
}
}
}

// Push the remaining preheaders, if any. This usually will happen if entry
// and exit blocks of lnum is same.
while (preHeadersList != nullptr)
{
BasicBlock* preHeaderBlock = preHeadersList->block;
JITDUMP(" -- " FMT_BB " (preheader of " FMT_LP ")\n", preHeaderBlock->bbNum,
optLoopEntry(preHeaderBlock)->bbNatLoopNum);
defExec.Push(preHeaderBlock);
preHeadersList = preHeadersList->next;
}

// If we didn't reach the entry block, give up and *just* push the entry block.
// If we didn't reach the entry block, give up and *just* push the entry block (and the pre-headers).
if (cur != pLoopDsc->lpEntry)
{
JITDUMP(" -- odd, we didn't reach entry from exit via dominators. Only considering hoisting in entry "
"block " FMT_BB "\n",
pLoopDsc->lpEntry->bbNum);
defExec.Reset();
pushAllPreheaders = true;
preHeadersList = existingPreHeaders;
}
}
else // More than one exit
Expand All @@ -6787,22 +6775,15 @@ bool Compiler::optHoistThisLoop(unsigned lnum, LoopHoistContext* hoistCtxt, Basi

JITDUMP(" Considering hoisting in entry block " FMT_BB " because " FMT_LP " has more than one exit\n",
pLoopDsc->lpEntry->bbNum, lnum);
pushAllPreheaders = true;
}

if (pushAllPreheaders)
// Push the remaining preheaders, if any.
for (; preHeadersList != nullptr; preHeadersList = preHeadersList->next)
{
// We will still push all the preheaders found.
BasicBlockList* preHeadersList = existingPreHeaders;

while (preHeadersList != nullptr)
{
BasicBlock* preHeaderBlock = preHeadersList->block;
JITDUMP(" -- " FMT_BB " (preheader of " FMT_LP ")\n", preHeaderBlock->bbNum,
optLoopEntry(preHeaderBlock)->bbNatLoopNum);
defExec.Push(preHeaderBlock);
preHeadersList = preHeadersList->next;
}
BasicBlock* preHeaderBlock = preHeadersList->block;
JITDUMP(" -- " FMT_BB " (preheader of " FMT_LP ")\n", preHeaderBlock->bbNum,
optLoopEntry(preHeaderBlock)->bbNatLoopNum);
defExec.Push(preHeaderBlock);
}

JITDUMP(" -- " FMT_BB " (entry block)\n", pLoopDsc->lpEntry->bbNum);
Expand Down Expand Up @@ -7219,36 +7200,38 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
Value& top = m_valueStack.TopRef();
assert(top.Node() == stmt->GetRootNode());

// hoist the top node?
if (top.m_hoistable)
{
m_compiler->optHoistCandidate(stmt->GetRootNode(), block, m_loopNum, m_hoistContext);
}
else
{
JITDUMP(" [%06u] not %s: %s\n", dspTreeID(top.Node()),
top.m_invariant ? "invariant" : "hoistable", top.m_failReason);
JITDUMP(" [%06u] %s: %s\n", dspTreeID(top.Node()),
top.m_invariant ? "not hoistable" : "not invariant", top.m_failReason);
}

m_valueStack.Reset();
}

// Only unconditionally executed blocks in the loop are visited (see optHoistThisLoop)
// so after we're done visiting the first block we need to assume the worst, that the
// blocks that are not visisted have side effects.
// blocks that are not visited have side effects.
m_beforeSideEffect = false;
}

fgWalkResult PreOrderVisit(GenTree** use, GenTree* user)
{
GenTree* node = *use;
JITDUMP("----- PreOrderVisit for [%06u] %s\n", dspTreeID(node), GenTree::OpName(node->OperGet()));
m_valueStack.Emplace(node);
return fgWalkResult::WALK_CONTINUE;
}

fgWalkResult PostOrderVisit(GenTree** use, GenTree* user)
{
GenTree* tree = *use;
JITDUMP("----- PostOrderVisit for [%06u]\n", dspTreeID(tree));
JITDUMP("----- PostOrderVisit for [%06u] %s\n", dspTreeID(tree), GenTree::OpName(tree->OperGet()));

if (tree->OperIsLocal())
{
Expand Down Expand Up @@ -7298,6 +7281,10 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
}
#endif

JITDUMP(" [%06u] %s: %s: %s\n", dspTreeID(tree), GenTree::OpName(tree->OperGet()),
top.m_invariant ? (top.m_hoistable ? "hoistable" : "not hoistable") : "not invariant",
top.m_failReason);

return fgWalkResult::WALK_CONTINUE;
}

Expand Down Expand Up @@ -7590,8 +7577,8 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
// should not hoist further children past it.
hasExcep = (tree->gtFlags & GTF_EXCEPT) != 0;
}
JITDUMP(" [%06u] not %s: %s\n", dspTreeID(value.Node()),
value.m_invariant ? "invariant" : "hoistable", value.m_failReason);
JITDUMP(" [%06u] %s: %s\n", dspTreeID(value.Node()),
value.m_invariant ? "not hoistable" : "not invariant", value.m_failReason);
}
else
{
Expand Down

0 comments on commit f28904a

Please sign in to comment.