From 893c9a476cdd7c7875910001d345605986f8ea1f Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 20 May 2024 10:25:23 -0400 Subject: [PATCH 1/6] wip --- src/coreclr/jit/compiler.h | 3 ++ src/coreclr/jit/fgbasic.cpp | 2 +- src/coreclr/jit/fgopt.cpp | 102 ++++++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index ff4e21444894fa..e409ef4a769bfa 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6095,6 +6095,9 @@ class Compiler bool fgReorderBlocks(bool useProfile); void fgDoReversePostOrderLayout(); void fgMoveColdBlocks(); + + template + void fgMoveBlocksToHottestSuccessors(); bool fgFuncletsAreCold(); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 538c49e530ecb1..829ff65cdc425e 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -6154,7 +6154,7 @@ BasicBlock* Compiler::fgNewBBFromTreeAfter( */ void Compiler::fgInsertBBbefore(BasicBlock* insertBeforeBlk, BasicBlock* newBlk) { - if (insertBeforeBlk->IsFirst()) + if (fgFirstBB == insertBeforeBlk) { newBlk->SetNext(fgFirstBB); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 88dd717fab6934..6d73b6e79bd602 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4535,6 +4535,104 @@ bool Compiler::fgReorderBlocks(bool useProfile) #pragma warning(pop) #endif +template +void Compiler::fgMoveBlocksToHottestSuccessors() +{ +#ifdef DEBUG + if (verbose) + { + printf("*************** In fgMoveBlocksToHottestSuccessors()\n"); + + printf("\nInitial BasicBlocks"); + fgDispBasicBlocks(verboseTrees); + printf("\n"); + } +#endif // DEBUG + + BasicBlock* next; + for (BasicBlock* block = fgFirstBB->Next(); block != fgFirstFuncletBB; block = next) + { + next = block->Next(); + + if (block->isRunRarely()) + { + continue; + } + + if (block->NumSucc() == 0) + { + continue; + } + + if (hasEH) + { + if (bbIsTryBeg(block) || bbIsHandlerBeg(block)) + { + continue; + } + } + + FlowEdge* likelySuccEdge = block->GetSuccEdge(0, this); + for (FlowEdge* const succEdge : block->SuccEdges(this)) + { + if (succEdge->getLikelihood() > likelySuccEdge->getLikelihood()) + { + likelySuccEdge = succEdge; + } + } + + BasicBlock* const likelySucc = likelySuccEdge->getDestinationBlock(); + if (likelySucc->IsFirst() || (block == likelySucc) || block->NextIs(likelySucc) || likelySucc->isRunRarely()) + { + continue; + } + + if (hasEH) + { + if (!BasicBlock::sameEHRegion(block, likelySucc) || bbIsTryBeg(likelySucc) || bbIsHandlerBeg(likelySucc)) + { + continue; + } + } + + bool isHeaviestEdge = true; + const weight_t likelySuccEdgeWeight = likelySuccEdge->getLikelyWeight(); + for (FlowEdge* const predEdge : likelySucc->PredEdges()) + { + if (predEdge == likelySuccEdge) + { + continue; + } + + if (predEdge->getLikelyWeight() >= likelySuccEdgeWeight) + { + isHeaviestEdge = false; + break; + } + } + + if (!isHeaviestEdge) + { + continue; + } + + assert(!block->IsFirst()); + FlowEdge* const fallthroughEdge = fgGetPredForBlock(block, block->Prev()); + if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= likelySuccEdgeWeight)) + { + continue; + } + + fgUnlinkBlock(block); + fgInsertBBbefore(likelySucc, block); + if (verbose) + { + fgDispBasicBlocks(); + } + } +} + + //----------------------------------------------------------------------------- // fgDoReversePostOrderLayout: Reorder blocks using a greedy RPO traversal. // @@ -4567,6 +4665,8 @@ void Compiler::fgDoReversePostOrderLayout() fgInsertBBafter(block, blockToMove); } + fgMoveBlocksToHottestSuccessors(); + return; } @@ -4645,6 +4745,8 @@ void Compiler::fgDoReversePostOrderLayout() } } + fgMoveBlocksToHottestSuccessors(); + // Fix up call-finally pairs // for (int i = 0; i < callFinallyPairs.Height(); i++) From 3d31c8be5140163be8e2686e2980c26561622413 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 20 May 2024 11:19:59 -0400 Subject: [PATCH 2/6] Add fgMoveBlocksToHottestSuccessors --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgopt.cpp | 41 +++++++++++++++++++++++++++++++------- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index e409ef4a769bfa..e7d3d5f24d13c2 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6095,7 +6095,7 @@ class Compiler bool fgReorderBlocks(bool useProfile); void fgDoReversePostOrderLayout(); void fgMoveColdBlocks(); - + template void fgMoveBlocksToHottestSuccessors(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 6d73b6e79bd602..e9d7638896cec0 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4535,6 +4535,12 @@ bool Compiler::fgReorderBlocks(bool useProfile) #pragma warning(pop) #endif +//----------------------------------------------------------------------------- +// fgMoveBlocksToHottestSuccessors: Try to create fallthrough between each block and its hottest successor. +// +// Template parameters: +// hasEH - If true, method has EH regions, so check that we don't try to move blocks in different regions +// template void Compiler::fgMoveBlocksToHottestSuccessors() { @@ -4549,29 +4555,40 @@ void Compiler::fgMoveBlocksToHottestSuccessors() } #endif // DEBUG + // Don't try to move the first block. + // Also, if we have a funclet region, don't bother reordering anything in it. + // BasicBlock* next; for (BasicBlock* block = fgFirstBB->Next(); block != fgFirstFuncletBB; block = next) { next = block->Next(); + // Don't bother trying to move cold blocks + // if (block->isRunRarely()) { continue; } - if (block->NumSucc() == 0) + // If this block doesn't have any successors, we have nothing to move it up to + // + if (block->NumSucc(this) == 0) { continue; } if (hasEH) { + // Don't move the beginning of an EH region + // if (bbIsTryBeg(block) || bbIsHandlerBeg(block)) { continue; } } + // Find this block's most likely successor + // FlowEdge* likelySuccEdge = block->GetSuccEdge(0, this); for (FlowEdge* const succEdge : block->SuccEdges(this)) { @@ -4581,6 +4598,11 @@ void Compiler::fgMoveBlocksToHottestSuccessors() } } + // We don't want to change the first block, so if the most likely successor is the first block, + // don't try moving this block before it. + // Also, there's nothing to do if the most likely successor is this block, or the next block. + // Finally, if the most likely successor is cold, don't bother moving this block up to it. + // BasicBlock* const likelySucc = likelySuccEdge->getDestinationBlock(); if (likelySucc->IsFirst() || (block == likelySucc) || block->NextIs(likelySucc) || likelySucc->isRunRarely()) { @@ -4589,13 +4611,18 @@ void Compiler::fgMoveBlocksToHottestSuccessors() if (hasEH) { + // Don't move blocks in different EH regions. + // Also, don't change the entry block of an EH region. + // if (!BasicBlock::sameEHRegion(block, likelySucc) || bbIsTryBeg(likelySucc) || bbIsHandlerBeg(likelySucc)) { continue; } } - bool isHeaviestEdge = true; + // Check if likelySuccEdge has the heaviest edge weight of likelySucc's predecessors + // + bool isHeaviestEdge = true; const weight_t likelySuccEdgeWeight = likelySuccEdge->getLikelyWeight(); for (FlowEdge* const predEdge : likelySucc->PredEdges()) { @@ -4616,6 +4643,9 @@ void Compiler::fgMoveBlocksToHottestSuccessors() continue; } + // block is the hottest predecessor of likelySucc, but before we move block, + // make sure any fallthrough we are breaking is worth losing + // assert(!block->IsFirst()); FlowEdge* const fallthroughEdge = fgGetPredForBlock(block, block->Prev()); if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= likelySuccEdgeWeight)) @@ -4623,16 +4653,13 @@ void Compiler::fgMoveBlocksToHottestSuccessors() continue; } + // Move block to before likelySucc + // fgUnlinkBlock(block); fgInsertBBbefore(likelySucc, block); - if (verbose) - { - fgDispBasicBlocks(); - } } } - //----------------------------------------------------------------------------- // fgDoReversePostOrderLayout: Reorder blocks using a greedy RPO traversal. // From 3ae278fdcbed068ca626677404262d8b11ec0c94 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 20 May 2024 11:24:26 -0400 Subject: [PATCH 3/6] Add comment --- src/coreclr/jit/fgopt.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index e9d7638896cec0..c894029b56c598 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4692,6 +4692,11 @@ void Compiler::fgDoReversePostOrderLayout() fgInsertBBafter(block, blockToMove); } + // The RPO established a good base layout, but in some cases, it might not place the hottest predecessor + // behind a block. + // If a block isn't placed before its most-likely successor, and it is that successor's hottest predecessor, + // try creating fallthrough between them. + // fgMoveBlocksToHottestSuccessors(); return; From f9d81a99aa6a9247ec58cc51512947e7671b5f9b Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 20 May 2024 12:09:38 -0400 Subject: [PATCH 4/6] Add loop head exception --- src/coreclr/jit/fgopt.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c894029b56c598..6a70dcc3e0544c 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4644,11 +4644,15 @@ void Compiler::fgMoveBlocksToHottestSuccessors() } // block is the hottest predecessor of likelySucc, but before we move block, - // make sure any fallthrough we are breaking is worth losing + // make sure any fallthrough we are breaking is worth losing. + // We make an exception for loop heads: If a loop is not entered from the top, + // the RPO-based layout may not place the loop head at the top of the loop, + // so fix that here. // assert(!block->IsFirst()); FlowEdge* const fallthroughEdge = fgGetPredForBlock(block, block->Prev()); - if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= likelySuccEdgeWeight)) + if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= likelySuccEdgeWeight) && + !block->HasFlag(BBF_LOOP_HEAD)) { continue; } From a5b9c21c88fe071cc22b489d4283902af65d95c3 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 20 May 2024 13:32:24 -0400 Subject: [PATCH 5/6] Revert "Add loop head exception" This reverts commit f9d81a99aa6a9247ec58cc51512947e7671b5f9b. --- src/coreclr/jit/fgopt.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 6a70dcc3e0544c..c894029b56c598 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4644,15 +4644,11 @@ void Compiler::fgMoveBlocksToHottestSuccessors() } // block is the hottest predecessor of likelySucc, but before we move block, - // make sure any fallthrough we are breaking is worth losing. - // We make an exception for loop heads: If a loop is not entered from the top, - // the RPO-based layout may not place the loop head at the top of the loop, - // so fix that here. + // make sure any fallthrough we are breaking is worth losing // assert(!block->IsFirst()); FlowEdge* const fallthroughEdge = fgGetPredForBlock(block, block->Prev()); - if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= likelySuccEdgeWeight) && - !block->HasFlag(BBF_LOOP_HEAD)) + if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= likelySuccEdgeWeight)) { continue; } From 608f1bd72f42275d82cad3f3ff838ab11d917e97 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 20 May 2024 17:17:07 -0400 Subject: [PATCH 6/6] Only move unconditional backward jumps --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgopt.cpp | 102 +++++++++++++------------------------ 2 files changed, 37 insertions(+), 67 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index e7d3d5f24d13c2..5f25dce0679922 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6097,7 +6097,7 @@ class Compiler void fgMoveColdBlocks(); template - void fgMoveBlocksToHottestSuccessors(); + void fgMoveBackwardJumpsToSuccessors(); bool fgFuncletsAreCold(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c894029b56c598..cf736d6f5d6bb9 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4536,18 +4536,18 @@ bool Compiler::fgReorderBlocks(bool useProfile) #endif //----------------------------------------------------------------------------- -// fgMoveBlocksToHottestSuccessors: Try to create fallthrough between each block and its hottest successor. +// fgMoveBackwardJumpsToSuccessors: Try to move backward unconditional jumps to fall into their successors. // // Template parameters: // hasEH - If true, method has EH regions, so check that we don't try to move blocks in different regions // template -void Compiler::fgMoveBlocksToHottestSuccessors() +void Compiler::fgMoveBackwardJumpsToSuccessors() { #ifdef DEBUG if (verbose) { - printf("*************** In fgMoveBlocksToHottestSuccessors()\n"); + printf("*************** In fgMoveBackwardJumpsToSuccessors()\n"); printf("\nInitial BasicBlocks"); fgDispBasicBlocks(verboseTrees); @@ -4555,6 +4555,10 @@ void Compiler::fgMoveBlocksToHottestSuccessors() } #endif // DEBUG + EnsureBasicBlockEpoch(); + BlockSet visitedBlocks(BlockSetOps::MakeEmpty(this)); + BlockSetOps::AddElemD(this, visitedBlocks, fgFirstBB->bbNum); + // Don't try to move the first block. // Also, if we have a funclet region, don't bother reordering anything in it. // @@ -4562,101 +4566,67 @@ void Compiler::fgMoveBlocksToHottestSuccessors() for (BasicBlock* block = fgFirstBB->Next(); block != fgFirstFuncletBB; block = next) { next = block->Next(); + BlockSetOps::AddElemD(this, visitedBlocks, block->bbNum); // Don't bother trying to move cold blocks // - if (block->isRunRarely()) + if (!block->KindIs(BBJ_ALWAYS) || block->isRunRarely()) { continue; } - // If this block doesn't have any successors, we have nothing to move it up to + // We will consider moving only backward jumps // - if (block->NumSucc(this) == 0) + BasicBlock* const target = block->GetTarget(); + if ((block == target) || !BlockSetOps::IsMember(this, visitedBlocks, target->bbNum)) { continue; } if (hasEH) { - // Don't move the beginning of an EH region + // Don't move blocks in different EH regions // - if (bbIsTryBeg(block) || bbIsHandlerBeg(block)) + if (!BasicBlock::sameEHRegion(block, target)) { continue; } - } - - // Find this block's most likely successor - // - FlowEdge* likelySuccEdge = block->GetSuccEdge(0, this); - for (FlowEdge* const succEdge : block->SuccEdges(this)) - { - if (succEdge->getLikelihood() > likelySuccEdge->getLikelihood()) - { - likelySuccEdge = succEdge; - } - } - // We don't want to change the first block, so if the most likely successor is the first block, - // don't try moving this block before it. - // Also, there's nothing to do if the most likely successor is this block, or the next block. - // Finally, if the most likely successor is cold, don't bother moving this block up to it. - // - BasicBlock* const likelySucc = likelySuccEdge->getDestinationBlock(); - if (likelySucc->IsFirst() || (block == likelySucc) || block->NextIs(likelySucc) || likelySucc->isRunRarely()) - { - continue; - } + // block and target are in the same try/handler regions, and target is behind block, + // so block cannot possibly be the start of the region. + // + assert(!bbIsTryBeg(block) && !bbIsHandlerBeg(block)); - if (hasEH) - { - // Don't move blocks in different EH regions. - // Also, don't change the entry block of an EH region. + // Don't change the entry block of an EH region // - if (!BasicBlock::sameEHRegion(block, likelySucc) || bbIsTryBeg(likelySucc) || bbIsHandlerBeg(likelySucc)) + if (bbIsTryBeg(target) || bbIsHandlerBeg(target)) { continue; } } - // Check if likelySuccEdge has the heaviest edge weight of likelySucc's predecessors + // We don't want to change the first block, so if the jump target is the first block, + // don't try moving this block before it. + // Also, if the target is cold, don't bother moving this block up to it. // - bool isHeaviestEdge = true; - const weight_t likelySuccEdgeWeight = likelySuccEdge->getLikelyWeight(); - for (FlowEdge* const predEdge : likelySucc->PredEdges()) - { - if (predEdge == likelySuccEdge) - { - continue; - } - - if (predEdge->getLikelyWeight() >= likelySuccEdgeWeight) - { - isHeaviestEdge = false; - break; - } - } - - if (!isHeaviestEdge) + if (target->IsFirst() || target->isRunRarely()) { continue; } - // block is the hottest predecessor of likelySucc, but before we move block, - // make sure any fallthrough we are breaking is worth losing + // If moving block will break up existing fallthrough behavior into target, make sure it's worth it // - assert(!block->IsFirst()); - FlowEdge* const fallthroughEdge = fgGetPredForBlock(block, block->Prev()); - if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= likelySuccEdgeWeight)) + FlowEdge* const fallthroughEdge = fgGetPredForBlock(target, target->Prev()); + if ((fallthroughEdge != nullptr) && + (fallthroughEdge->getLikelyWeight() >= block->GetTargetEdge()->getLikelyWeight())) { continue; } - // Move block to before likelySucc + // Move block to before target // fgUnlinkBlock(block); - fgInsertBBbefore(likelySucc, block); + fgInsertBBbefore(target, block); } } @@ -4692,12 +4662,12 @@ void Compiler::fgDoReversePostOrderLayout() fgInsertBBafter(block, blockToMove); } - // The RPO established a good base layout, but in some cases, it might not place the hottest predecessor - // behind a block. - // If a block isn't placed before its most-likely successor, and it is that successor's hottest predecessor, - // try creating fallthrough between them. + // The RPO established a good base layout, but in some cases, it might produce a subpar layout for loops. + // In particular, it may place the loop head after the loop exit, creating unnecessary branches. + // Fix this by moving unconditional backward jumps up to their targets, + // increasing the likelihood that the loop exit block is the last block in the loop. // - fgMoveBlocksToHottestSuccessors(); + fgMoveBackwardJumpsToSuccessors(); return; } @@ -4777,7 +4747,7 @@ void Compiler::fgDoReversePostOrderLayout() } } - fgMoveBlocksToHottestSuccessors(); + fgMoveBackwardJumpsToSuccessors(); // Fix up call-finally pairs //