Skip to content

Commit 68f3edc

Browse files
JIT: Move backward jumps to before their successors after RPO-based layout (dotnet#102461)
Part of dotnet#93020. In dotnet#102343, we noticed the RPO-based layout sometimes makes suboptimal decisions in terms of placing a block's hottest predecessor before it -- in particular, this affects loops that aren't entered at the top. To address this, after establishing a baseline RPO layout, fgMoveBackwardJumpsToSuccessors will try to move backward unconditional jumps to right behind their targets to create fallthrough, if the predecessor block is sufficiently hot.
1 parent 99274eb commit 68f3edc

File tree

3 files changed

+108
-1
lines changed

3 files changed

+108
-1
lines changed

src/coreclr/jit/compiler.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6104,6 +6104,9 @@ class Compiler
61046104
void fgDoReversePostOrderLayout();
61056105
void fgMoveColdBlocks();
61066106

6107+
template <bool hasEH>
6108+
void fgMoveBackwardJumpsToSuccessors();
6109+
61076110
bool fgFuncletsAreCold();
61086111

61096112
PhaseStatus fgDetermineFirstColdBlock();

src/coreclr/jit/fgbasic.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6154,7 +6154,7 @@ BasicBlock* Compiler::fgNewBBFromTreeAfter(
61546154
*/
61556155
void Compiler::fgInsertBBbefore(BasicBlock* insertBeforeBlk, BasicBlock* newBlk)
61566156
{
6157-
if (insertBeforeBlk->IsFirst())
6157+
if (fgFirstBB == insertBeforeBlk)
61586158
{
61596159
newBlk->SetNext(fgFirstBB);
61606160

src/coreclr/jit/fgopt.cpp

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4535,6 +4535,101 @@ bool Compiler::fgReorderBlocks(bool useProfile)
45354535
#pragma warning(pop)
45364536
#endif
45374537

4538+
//-----------------------------------------------------------------------------
4539+
// fgMoveBackwardJumpsToSuccessors: Try to move backward unconditional jumps to fall into their successors.
4540+
//
4541+
// Template parameters:
4542+
// hasEH - If true, method has EH regions, so check that we don't try to move blocks in different regions
4543+
//
4544+
template <bool hasEH>
4545+
void Compiler::fgMoveBackwardJumpsToSuccessors()
4546+
{
4547+
#ifdef DEBUG
4548+
if (verbose)
4549+
{
4550+
printf("*************** In fgMoveBackwardJumpsToSuccessors()\n");
4551+
4552+
printf("\nInitial BasicBlocks");
4553+
fgDispBasicBlocks(verboseTrees);
4554+
printf("\n");
4555+
}
4556+
#endif // DEBUG
4557+
4558+
EnsureBasicBlockEpoch();
4559+
BlockSet visitedBlocks(BlockSetOps::MakeEmpty(this));
4560+
BlockSetOps::AddElemD(this, visitedBlocks, fgFirstBB->bbNum);
4561+
4562+
// Don't try to move the first block.
4563+
// Also, if we have a funclet region, don't bother reordering anything in it.
4564+
//
4565+
BasicBlock* next;
4566+
for (BasicBlock* block = fgFirstBB->Next(); block != fgFirstFuncletBB; block = next)
4567+
{
4568+
next = block->Next();
4569+
BlockSetOps::AddElemD(this, visitedBlocks, block->bbNum);
4570+
4571+
// Don't bother trying to move cold blocks
4572+
//
4573+
if (!block->KindIs(BBJ_ALWAYS) || block->isRunRarely())
4574+
{
4575+
continue;
4576+
}
4577+
4578+
// We will consider moving only backward jumps
4579+
//
4580+
BasicBlock* const target = block->GetTarget();
4581+
if ((block == target) || !BlockSetOps::IsMember(this, visitedBlocks, target->bbNum))
4582+
{
4583+
continue;
4584+
}
4585+
4586+
if (hasEH)
4587+
{
4588+
// Don't move blocks in different EH regions
4589+
//
4590+
if (!BasicBlock::sameEHRegion(block, target))
4591+
{
4592+
continue;
4593+
}
4594+
4595+
// block and target are in the same try/handler regions, and target is behind block,
4596+
// so block cannot possibly be the start of the region.
4597+
//
4598+
assert(!bbIsTryBeg(block) && !bbIsHandlerBeg(block));
4599+
4600+
// Don't change the entry block of an EH region
4601+
//
4602+
if (bbIsTryBeg(target) || bbIsHandlerBeg(target))
4603+
{
4604+
continue;
4605+
}
4606+
}
4607+
4608+
// We don't want to change the first block, so if the jump target is the first block,
4609+
// don't try moving this block before it.
4610+
// Also, if the target is cold, don't bother moving this block up to it.
4611+
//
4612+
if (target->IsFirst() || target->isRunRarely())
4613+
{
4614+
continue;
4615+
}
4616+
4617+
// If moving block will break up existing fallthrough behavior into target, make sure it's worth it
4618+
//
4619+
FlowEdge* const fallthroughEdge = fgGetPredForBlock(target, target->Prev());
4620+
if ((fallthroughEdge != nullptr) &&
4621+
(fallthroughEdge->getLikelyWeight() >= block->GetTargetEdge()->getLikelyWeight()))
4622+
{
4623+
continue;
4624+
}
4625+
4626+
// Move block to before target
4627+
//
4628+
fgUnlinkBlock(block);
4629+
fgInsertBBbefore(target, block);
4630+
}
4631+
}
4632+
45384633
//-----------------------------------------------------------------------------
45394634
// fgDoReversePostOrderLayout: Reorder blocks using a greedy RPO traversal.
45404635
//
@@ -4567,6 +4662,13 @@ void Compiler::fgDoReversePostOrderLayout()
45674662
fgInsertBBafter(block, blockToMove);
45684663
}
45694664

4665+
// The RPO established a good base layout, but in some cases, it might produce a subpar layout for loops.
4666+
// In particular, it may place the loop head after the loop exit, creating unnecessary branches.
4667+
// Fix this by moving unconditional backward jumps up to their targets,
4668+
// increasing the likelihood that the loop exit block is the last block in the loop.
4669+
//
4670+
fgMoveBackwardJumpsToSuccessors</* hasEH */ false>();
4671+
45704672
return;
45714673
}
45724674

@@ -4645,6 +4747,8 @@ void Compiler::fgDoReversePostOrderLayout()
46454747
}
46464748
}
46474749

4750+
fgMoveBackwardJumpsToSuccessors</* hasEH */ true>();
4751+
46484752
// Fix up call-finally pairs
46494753
//
46504754
for (int i = 0; i < callFinallyPairs.Height(); i++)

0 commit comments

Comments
 (0)