From 94298cca19dc921d93c8eeabc5150d06f51473ab Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 30 May 2024 18:43:25 -0400 Subject: [PATCH 1/6] One loop in fgMoveBackwardJumpsToSuccessors --- src/coreclr/jit/fgopt.cpp | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index de2b6a8f37c936..a907e53d107f1e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4557,33 +4557,26 @@ void Compiler::fgMoveBackwardJumpsToSuccessors() } #endif // DEBUG - // Compact blocks before trying to move any jumps. - // This can unlock more opportunities for fallthrough behavior. + EnsureBasicBlockEpoch(); + BlockSet visitedBlocks(BlockSetOps::MakeEmpty(this)); + BlockSetOps::AddElemD(this, visitedBlocks, fgFirstBB->bbNum); + + // If we have a funclet region, don't bother reordering anything in it. // - for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB;) + BasicBlock* next; + for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB; block = next) { if (fgCanCompactBlocks(block, block->Next())) { - // We haven't fixed EH information yet, so don't do any correctness checks here + // Compact blocks before trying to move any jumps. + // This can unlock more opportunities for fallthrough behavior. + // We haven't fixed EH information yet, so don't do any correctness checks here. // fgCompactBlocks(block, block->Next() DEBUGARG(/* doDebugCheck */ false)); + next = block; + continue; } - else - { - block = block->Next(); - } - } - - 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. - // - BasicBlock* next; - for (BasicBlock* block = fgFirstBB->Next(); block != fgFirstFuncletBB; block = next) - { next = block->Next(); BlockSetOps::AddElemD(this, visitedBlocks, block->bbNum); From 8108a9a47cf2a5e32e83e565feca3b33a46d8a48 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 31 May 2024 12:32:01 -0400 Subject: [PATCH 2/6] Move forward jumps --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgopt.cpp | 127 ++++++++++++++++++++++++++----------- 2 files changed, 91 insertions(+), 38 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index dd92e05f464afc..1f964ca4dbd074 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6134,7 +6134,7 @@ class Compiler void fgMoveColdBlocks(); template - void fgMoveBackwardJumpsToSuccessors(); + void fgMoveHotJumps(); bool fgFuncletsAreCold(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index a907e53d107f1e..ed7155d810c86e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4538,18 +4538,18 @@ bool Compiler::fgReorderBlocks(bool useProfile) #endif //----------------------------------------------------------------------------- -// fgMoveBackwardJumpsToSuccessors: Try to move backward unconditional jumps to fall into their successors. +// fgMoveHotJumps: Try to move jumps to fall into their successors, if the jump is sufficiently hot. // // 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::fgMoveBackwardJumpsToSuccessors() +void Compiler::fgMoveHotJumps() { #ifdef DEBUG if (verbose) { - printf("*************** In fgMoveBackwardJumpsToSuccessors()\n"); + printf("*************** In fgMoveHotJumps()\n"); printf("\nInitial BasicBlocks"); fgDispBasicBlocks(verboseTrees); @@ -4582,63 +4582,121 @@ void Compiler::fgMoveBackwardJumpsToSuccessors() // Don't bother trying to move cold blocks // - if (!block->KindIs(BBJ_ALWAYS) || block->isRunRarely()) + if (block->isRunRarely()) { continue; } - // We will consider moving only backward jumps + FlowEdge* targetEdge; + + if (block->KindIs(BBJ_ALWAYS)) + { + targetEdge = block->GetTargetEdge(); + } + else if (block->KindIs(BBJ_COND)) + { + // Consider conditional block's most likely branch for moving + // + targetEdge = (block->GetTrueEdge()->getLikelihood() > 0.5) ? block->GetTrueEdge() : block->GetFalseEdge(); + } + else + { + // Don't consider other block kinds + // + continue; + } + + BasicBlock* const target = targetEdge->getDestinationBlock(); + + // Check for single-block loop and fallthrough cases // - BasicBlock* const target = block->GetTarget(); - if ((block == target) || !BlockSetOps::IsMember(this, visitedBlocks, target->bbNum)) + if ((block == target) || block->NextIs(target)) { continue; } - if (hasEH) + if (target->isRunRarely()) { - // Don't move blocks in different EH regions + // block is not run rarely, so why is target marked as run rarely? // - if (!BasicBlock::sameEHRegion(block, target)) + continue; + } + + const bool isBackwardJump = BlockSetOps::IsMember(this, visitedBlocks, target->bbNum); + + if (isBackwardJump) + { + // We don't want to change the first block, so if block is a backward jump to the first block, + // don't try moving block before it. + // + if (target->IsFirst()) { 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)); - - // Don't change the entry block of an EH region - // - if (bbIsTryBeg(target) || bbIsHandlerBeg(target)) + if (block->KindIs(BBJ_COND)) { + // This could be a loop exit, so don't bother moving this block up + // continue; } } - // 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. - // - if (target->IsFirst() || target->isRunRarely()) + if (hasEH) { - continue; + // Don't move blocks in different EH regions + // + if (!BasicBlock::sameEHRegion(block, target)) + { + continue; + } + + if (isBackwardJump) + { + // 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)); + + // Don't change the entry block of an EH region + // + if (bbIsTryBeg(target) || bbIsHandlerBeg(target)) + { + continue; + } + } + else + { + // block and target are in the same try/handler regions, and block is behind target, + // so target cannot possibly be the start of the region. + // + assert(!bbIsTryBeg(target) && !bbIsHandlerBeg(target)); + } } // If moving block will break up existing fallthrough behavior into target, make sure it's worth it // FlowEdge* const fallthroughEdge = fgGetPredForBlock(target, target->Prev()); - if ((fallthroughEdge != nullptr) && - (fallthroughEdge->getLikelyWeight() >= block->GetTargetEdge()->getLikelyWeight())) + if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= targetEdge->getLikelyWeight())) { continue; } - // Move block to before target - // - fgUnlinkBlock(block); - fgInsertBBbefore(target, block); + if (isBackwardJump) + { + // Move block to before target + // + fgUnlinkBlock(block); + fgInsertBBbefore(target, block); + } + else + { + // Move target up to block + // + fgUnlinkBlock(target); + fgInsertBBafter(block, target); + next = target; + } } } @@ -4674,12 +4732,7 @@ void Compiler::fgDoReversePostOrderLayout() fgInsertBBafter(block, blockToMove); } - // 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. - // - fgMoveBackwardJumpsToSuccessors(); + fgMoveHotJumps(); return; } @@ -4768,7 +4821,7 @@ void Compiler::fgDoReversePostOrderLayout() fgInsertBBafter(pair.callFinally, pair.callFinallyRet); } - fgMoveBackwardJumpsToSuccessors(); + fgMoveHotJumps(); // The RPO won't change the entry blocks of any EH regions, but reordering can change the last block in a region // (for example, by pushing throw blocks unreachable via normal flow to the end of the region). From 2b43273e71efa63cdccc4c436df4881731e962f7 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 3 Jun 2024 17:17:19 -0400 Subject: [PATCH 3/6] Fix x86 EH failures --- src/coreclr/jit/fgopt.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index ed7155d810c86e..43498cea9592cc 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4689,6 +4689,14 @@ void Compiler::fgMoveHotJumps() fgUnlinkBlock(block); fgInsertBBbefore(target, block); } + else if (hasEH && target->isBBCallFinallyPair()) + { + // target is a call-finally pair, so move the pair up to block + // + fgUnlinkRange(target, target->Next()); + fgMoveBlocksAfter(target, target->Next(), block); + next = target->Next(); + } else { // Move target up to block From b202424c6b4be1c2b5ec53e3bcfda6636100e174 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 14 Jun 2024 18:19:45 -0400 Subject: [PATCH 4/6] Move unlikely successor of BBJ_COND block if forward jump --- src/coreclr/jit/fgopt.cpp | 65 +++++++++++++++++++++++++++------------ 1 file changed, 45 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 43498cea9592cc..e06e5454b733e3 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4588,16 +4588,27 @@ void Compiler::fgMoveHotJumps() } FlowEdge* targetEdge; + FlowEdge* unlikelyEdge; if (block->KindIs(BBJ_ALWAYS)) { - targetEdge = block->GetTargetEdge(); + targetEdge = block->GetTargetEdge(); + unlikelyEdge = nullptr; } else if (block->KindIs(BBJ_COND)) { // Consider conditional block's most likely branch for moving // - targetEdge = (block->GetTrueEdge()->getLikelihood() > 0.5) ? block->GetTrueEdge() : block->GetFalseEdge(); + if (block->GetTrueEdge()->getLikelihood() > 0.5) + { + targetEdge = block->GetTrueEdge(); + unlikelyEdge = block->GetFalseEdge(); + } + else + { + targetEdge = block->GetFalseEdge(); + unlikelyEdge = block->GetTrueEdge(); + } } else { @@ -4606,23 +4617,8 @@ void Compiler::fgMoveHotJumps() continue; } - BasicBlock* const target = targetEdge->getDestinationBlock(); - - // Check for single-block loop and fallthrough cases - // - if ((block == target) || block->NextIs(target)) - { - continue; - } - - if (target->isRunRarely()) - { - // block is not run rarely, so why is target marked as run rarely? - // - continue; - } - - const bool isBackwardJump = BlockSetOps::IsMember(this, visitedBlocks, target->bbNum); + BasicBlock* target = targetEdge->getDestinationBlock(); + bool isBackwardJump = BlockSetOps::IsMember(this, visitedBlocks, target->bbNum); if (isBackwardJump) { @@ -4636,12 +4632,41 @@ void Compiler::fgMoveHotJumps() if (block->KindIs(BBJ_COND)) { - // This could be a loop exit, so don't bother moving this block up + // This could be a loop exit, so don't bother moving this block up. + // Instead, try moving the unlikely target up to create fallthrough. // + targetEdge = unlikelyEdge; + target = targetEdge->getDestinationBlock(); + isBackwardJump = BlockSetOps::IsMember(this, visitedBlocks, target->bbNum); + + if (isBackwardJump) + { + continue; + } + } + // Check for single-block loop case + // + else if (block == target) + { continue; } } + // Check if block already falls into target + // + if (block->NextIs(target)) + { + continue; + } + + if (target->isRunRarely()) + { + // TODO: If target is block's most-likely successor, and block is not rarely-run, + // perhaps the profile data is misleading? Consider moving target anyway. + // + continue; + } + if (hasEH) { // Don't move blocks in different EH regions From 701682b44adaf054ef45cc0e0b6bf8823fff0067 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 14 Jun 2024 22:39:43 -0400 Subject: [PATCH 5/6] Check for cold block weights --- src/coreclr/jit/fgopt.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 89dcafd78bd33f..73f12513c163fb 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4582,7 +4582,7 @@ void Compiler::fgMoveHotJumps() // Don't bother trying to move cold blocks // - if (block->isRunRarely()) + if (block->isBBWeightCold()) { continue; } @@ -4659,10 +4659,10 @@ void Compiler::fgMoveHotJumps() continue; } - if (target->isRunRarely()) + if (target->isBBWeightCold()) { - // TODO: If target is block's most-likely successor, and block is not rarely-run, - // perhaps the profile data is misleading? Consider moving target anyway. + // If target is block's most-likely successor, and block is not rarely-run, + // perhaps the profile data is misleading, and we need to run profile repair? // continue; } From a5c528e95797684ce5092bc477cc0e8cd8be0457 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 14 Jun 2024 22:46:38 -0400 Subject: [PATCH 6/6] Fix build --- src/coreclr/jit/fgopt.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 73f12513c163fb..7f0c0f2e342a5e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4582,7 +4582,7 @@ void Compiler::fgMoveHotJumps() // Don't bother trying to move cold blocks // - if (block->isBBWeightCold()) + if (block->isBBWeightCold(this)) { continue; } @@ -4659,7 +4659,7 @@ void Compiler::fgMoveHotJumps() continue; } - if (target->isBBWeightCold()) + if (target->isBBWeightCold(this)) { // If target is block's most-likely successor, and block is not rarely-run, // perhaps the profile data is misleading, and we need to run profile repair?