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

Jump to bottom

Improve loop inversion #52347

Merged
merged 3 commits into from
May 7, 2021
Merged

Improve loop inversion #52347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7eb39d
Improve loop inversion
BruceForstall May 5, 2021
b23e26e
Formatting
BruceForstall May 5, 2021
8c7ed35
Updates
BruceForstall May 6, 2021
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
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6516,7 +6516,7 @@ class Compiler
// loop nested in "loopInd" that shares the same head as "loopInd".
void optUpdateLoopHead(unsigned loopInd, BasicBlock* from, BasicBlock* to);

void optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap);
void optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap, const bool updatePreds = false);

// Marks the containsCall information to "lnum" and any parent loops.
void AddContainsCallAllContainingLoops(unsigned lnum);
Expand Down
127 changes: 87 additions & 40 deletions src/coreclr/jit/optimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2613,11 +2613,14 @@ void Compiler::optIdentifyLoopsForAlignment()
// Updates the successors of `blk`: if `blk2` is a branch successor of `blk`, and there is a mapping
// for `blk2->blk3` in `redirectMap`, change `blk` so that `blk3` is this branch successor.
//
// Note that fall-through successors are not modified, including predecessor lists.
//
// Arguments:
// blk - block to redirect
// redirectMap - block->block map specifying how the `blk` target will be redirected.
// updatePreds - if `true`, update the predecessor lists to match.
//
void Compiler::optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap)
void Compiler::optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap, const bool updatePreds)
{
BasicBlock* newJumpDest = nullptr;
switch (blk->bbJumpKind)
Expand All @@ -2638,6 +2641,11 @@ void Compiler::optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap)
// All of these have a single jump destination to update.
if (redirectMap->Lookup(blk->bbJumpDest, &newJumpDest))
{
if (updatePreds)
{
fgRemoveRefPred(blk->bbJumpDest, blk);
fgAddRefPred(newJumpDest, blk);
}
blk->bbJumpDest = newJumpDest;
}
break;
Expand All @@ -2650,7 +2658,11 @@ void Compiler::optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap)
BasicBlock* switchDest = blk->bbJumpSwt->bbsDstTab[i];
if (redirectMap->Lookup(switchDest, &newJumpDest))
{
switchDest = newJumpDest;
if (updatePreds)
{
fgRemoveRefPred(switchDest, blk);
fgAddRefPred(newJumpDest, blk);
}
blk->bbJumpSwt->bbsDstTab[i] = newJumpDest;
redirected = true;
}
Expand Down Expand Up @@ -4188,21 +4200,14 @@ void Compiler::optInvertWhileLoop(BasicBlock* block)
assert(opts.OptimizationEnabled());
assert(compCodeOpt() != SMALL_CODE);

/* Does the BB end with an unconditional jump? */
// Does the BB end with an unconditional jump?

if (block->bbJumpKind != BBJ_ALWAYS || (block->bbFlags & BBF_KEEP_BBJ_ALWAYS))
{
// It can't be one of the ones we use for our exception magic
return;
}

// block can't be the scratch bb, since we prefer to keep flow
// out of the scratch bb as BBJ_ALWAYS or BBJ_NONE.
if (fgBBisScratch(block))
{
return;
}

// Get hold of the jump target
BasicBlock* bTest = block->bbJumpDest;

Expand All @@ -4221,14 +4226,16 @@ void Compiler::optInvertWhileLoop(BasicBlock* block)
// Since test is a BBJ_COND it will have a bbNext
noway_assert(bTest->bbNext != nullptr);

// 'block' must be in the same try region as the condition, since we're going to insert
// a duplicated condition in 'block', and the condition might include exception throwing code.
if (!BasicBlock::sameTryRegion(block, bTest))
// 'block' must be in the same try region as the condition, since we're going to insert a duplicated condition
// in a new block after 'block', and the condition might include exception throwing code.
// On non-funclet platforms (x86), the catch exit is a BBJ_ALWAYS, but we don't want that to
// be considered as the head of a loop, so also disallow different handler regions.
if (!BasicBlock::sameEHRegion(block, bTest))
{
return;
}

// We're going to change 'block' to branch to bTest->bbNext, so that also better be in the
// The duplicated condition block will branch to bTest->bbNext, so that also better be in the
// same try region (or no try region) to avoid generating illegal flow.
BasicBlock* bTestNext = bTest->bbNext;
if (bTestNext->hasTryIndex() && !BasicBlock::sameTryRegion(block, bTestNext))
Expand Down Expand Up @@ -4411,7 +4418,12 @@ void Compiler::optInvertWhileLoop(BasicBlock* block)

bool foundCondTree = false;

// Clone each statement in bTest and append to block.
// Create a new block after `block` to put the copied condition code.
block->bbJumpKind = BBJ_NONE;
block->bbJumpDest = nullptr;
BasicBlock* bNewCond = fgNewBBafter(BBJ_COND, block, /*extendRegion*/ true);

// Clone each statement in bTest and append to bNewCond.
for (Statement* stmt : bTest->Statements())
{
GenTree* originalTree = stmt->GetRootNode();
Expand Down Expand Up @@ -4439,7 +4451,7 @@ void Compiler::optInvertWhileLoop(BasicBlock* block)
gtReverseCond(clonedCompareTree);
}

Statement* clonedStmt = fgNewStmtAtEnd(block, clonedTree);
Statement* clonedStmt = fgNewStmtAtEnd(bNewCond, clonedTree);

if (opts.compDbgInfo)
{
Expand All @@ -4456,24 +4468,59 @@ void Compiler::optInvertWhileLoop(BasicBlock* block)
// this is a conservative guess.
if (auto copyFlags = bTest->bbFlags & (BBF_HAS_IDX_LEN | BBF_HAS_NULLCHECK | BBF_HAS_NEWOBJ | BBF_HAS_NEWARRAY))
{
block->bbFlags |= copyFlags;
bNewCond->bbFlags |= copyFlags;
}

/* Change the block to end with a conditional jump */

block->bbJumpKind = BBJ_COND;
block->bbJumpDest = bTest->bbNext;
bNewCond->bbJumpDest = bTest->bbNext;
bNewCond->inheritWeight(block);

/* Update bbRefs and bbPreds for 'block->bbNext' 'bTest' and 'bTest->bbNext' */
// Update bbRefs and bbPreds for 'bNewCond', 'bNewCond->bbNext' 'bTest' and 'bTest->bbNext'.

fgAddRefPred(block->bbNext, block);
fgAddRefPred(bNewCond, block);
fgAddRefPred(bNewCond->bbNext, bNewCond);

fgRemoveRefPred(bTest, block);
fgAddRefPred(bTest->bbNext, block);
fgAddRefPred(bTest->bbNext, bNewCond);

// Move all predecessor edges that look like loop entry edges to point to the new cloned condition
// block, not the existing condition block. The idea is that if we only move `block` to point to
// `bNewCond`, but leave other `bTest` predecessors still pointing to `bTest`, when we eventually
// recognize loops, the loop will appear to have multiple entries, which will prevent optimization.
// We don't have loops yet, but blocks should be in increasing lexical numbered order, so use that
// as the proxy for predecessors that are "in" versus "out" of the potential loop. Note that correctness
// is maintained no matter which condition block we point to, but we'll lose optimization potential
// (and create spaghetti code) if we get it wrong.

BlockToBlockMap blockMap(getAllocator(CMK_LoopOpt));
bool blockMapInitialized = false;

unsigned loopFirstNum = bNewCond->bbNext->bbNum;
unsigned loopBottomNum = bTest->bbNum;
for (flowList* pred = bTest->bbPreds; pred != nullptr; pred = pred->flNext)
{
BasicBlock* predBlock = pred->getBlock();
unsigned bNum = predBlock->bbNum;
if ((loopFirstNum <= bNum) && (bNum <= loopBottomNum))
{
// Looks like the predecessor is from within the potential loop; skip it.
continue;
}

if (!blockMapInitialized)
{
blockMapInitialized = true;
blockMap.Set(bTest, bNewCond);
}

// Redirect the predecessor to the new block.
JITDUMP("Redirecting non-loop " FMT_BB " -> " FMT_BB " to " FMT_BB " -> " FMT_BB "\n", predBlock->bbNum,
bTest->bbNum, predBlock->bbNum, bNewCond->bbNum);
optRedirectBlock(predBlock, &blockMap, /*updatePreds*/ true);
Copy link
Member

Choose a reason for hiding this comment

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

Since we expect optRedirectBlock to actually do something, I wonder if it should return a bool indicating that it made changes..?

Or, perhaps verify after all this redirecting that the block and the cloned block now have the expected numbers of preds?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm... I think the only way it could fail to redirect is if the predecessor is BBJ_NONE, which shouldn't occur. I suppose your suggestion is just to assert this. (Other types like BBJ_THROW, BBJ_RETURN, should never be predecessors.)

It turns out if it ends up not redirecting the pred, it's not fatal anyway, it's just sub-optimal.

}

// If we have profile data for all blocks and we know that we are cloning the
// bTest block into block and thus changing the control flow from block so
// that it no longer goes directly to bTest anymore, we have to adjust
// `bTest` block into `bNewCond` and thus changing the control flow from `block` so
// that it no longer goes directly to `bTest` anymore, we have to adjust
// various weights.
//
if (allProfileWeightsAreValid)
Expand Down Expand Up @@ -4521,33 +4568,33 @@ void Compiler::optInvertWhileLoop(BasicBlock* block)
BasicBlock::weight_t const blockToNextWeight = weightBlock * blockToNextLikelihood;
BasicBlock::weight_t const blockToAfterWeight = weightBlock * blockToAfterLikelihood;

flowList* const edgeBlockToNext = fgGetPredForBlock(block->bbNext, block);
flowList* const edgeBlockToAfter = fgGetPredForBlock(block->bbJumpDest, block);
flowList* const edgeBlockToNext = fgGetPredForBlock(bNewCond->bbNext, bNewCond);
flowList* const edgeBlockToAfter = fgGetPredForBlock(bNewCond->bbJumpDest, bNewCond);

JITDUMP("Setting weight of " FMT_BB " -> " FMT_BB " to " FMT_WT " (enter loop)\n", block->bbNum,
block->bbNext->bbNum, blockToNextWeight);
JITDUMP("Setting weight of " FMT_BB " -> " FMT_BB " to " FMT_WT " (avoid loop)\n", block->bbNum,
block->bbJumpDest->bbNum, blockToAfterWeight);
JITDUMP("Setting weight of " FMT_BB " -> " FMT_BB " to " FMT_WT " (enter loop)\n", bNewCond->bbNum,
bNewCond->bbNext->bbNum, blockToNextWeight);
JITDUMP("Setting weight of " FMT_BB " -> " FMT_BB " to " FMT_WT " (avoid loop)\n", bNewCond->bbNum,
bNewCond->bbJumpDest->bbNum, blockToAfterWeight);

edgeBlockToNext->setEdgeWeights(blockToNextWeight, blockToNextWeight, block->bbNext);
edgeBlockToAfter->setEdgeWeights(blockToAfterWeight, blockToAfterWeight, block->bbJumpDest);
edgeBlockToNext->setEdgeWeights(blockToNextWeight, blockToNextWeight, bNewCond->bbNext);
edgeBlockToAfter->setEdgeWeights(blockToAfterWeight, blockToAfterWeight, bNewCond->bbJumpDest);

#ifdef DEBUG
// Verify profile for the two target blocks is consistent.
//
fgDebugCheckIncomingProfileData(block->bbNext);
fgDebugCheckIncomingProfileData(block->bbJumpDest);
fgDebugCheckIncomingProfileData(bNewCond->bbNext);
fgDebugCheckIncomingProfileData(bNewCond->bbJumpDest);
#endif // DEBUG
}

#ifdef DEBUG
if (verbose)
{
printf("\nDuplicated loop exit block at " FMT_BB " for loop (" FMT_BB " - " FMT_BB ")\n", block->bbNum,
block->bbNext->bbNum, bTest->bbNum);
printf("\nDuplicated loop exit block at " FMT_BB " for loop (" FMT_BB " - " FMT_BB ")\n", bNewCond->bbNum,
bNewCond->bbNext->bbNum, bTest->bbNum);
printf("Estimated code size expansion is %d\n", estDupCostSz);

fgDumpBlock(block);
fgDumpBlock(bNewCond);
fgDumpBlock(bTest);
}
#endif // DEBUG
Expand Down Expand Up @@ -4583,7 +4630,7 @@ PhaseStatus Compiler::optInvertLoops()
//
if (block->bbWeight == BB_ZERO_WEIGHT)
{
/* Zero weighted block can't have a LOOP_HEAD flag */
// Zero weighted block can't have a LOOP_HEAD flag
noway_assert(block->isLoopHead() == false);
continue;
}
Expand Down