From db909590c76947c1ea8972d7bcadce932b58d908 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 17 Jun 2024 21:36:32 -0400 Subject: [PATCH 1/7] Use editing iterator --- src/coreclr/jit/fgopt.cpp | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 305af19b91df3..e0df5c7e27b4f 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -999,17 +999,9 @@ void Compiler::fgCompactBlocks(BasicBlock* block, BasicBlock* bNext DEBUGARG(boo JITDUMP("Second block has %u other incoming edges\n", bNext->countOfInEdges()); assert(block->isEmpty()); - // Retarget all the other edges incident on bNext. Do this - // in two passes as we can't both walk and modify the pred list. - // - ArrayStack preds(getAllocator(CMK_BasicBlock), bNext->countOfInEdges()); - for (BasicBlock* const predBlock : bNext->PredBlocks()) - { - preds.Push(predBlock); - } - while (preds.Height() > 0) + // Retarget all the other edges incident on bNext + for (BasicBlock* const predBlock : bNext->PredBlocksEditing()) { - BasicBlock* const predBlock = preds.Pop(); fgReplaceJumpTarget(predBlock, bNext, block); } } From 76f4d2963a0416efa34d28b0f5edbc9bbcd71d19 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 18 Jun 2024 00:34:37 -0400 Subject: [PATCH 2/7] Enable compaction of block target --- src/coreclr/jit/compiler.h | 4 ++-- src/coreclr/jit/fgdiagnostic.cpp | 2 +- src/coreclr/jit/fgopt.cpp | 37 +++++++++++++++++++---------- src/coreclr/jit/helperexpansion.cpp | 12 +++++----- src/coreclr/jit/optimizebools.cpp | 4 ++-- 5 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 8ccaeec161f06..a62825495b151 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6083,9 +6083,9 @@ class Compiler void fgPrepareCallFinallyRetForRemoval(BasicBlock* block); - bool fgCanCompactBlocks(BasicBlock* block, BasicBlock* bNext); + bool fgCanCompactBlocks(BasicBlock* block); - void fgCompactBlocks(BasicBlock* block, BasicBlock* bNext DEBUGARG(bool doDebugCheck = true)); + void fgCompactBlocks(BasicBlock* block DEBUGARG(bool doDebugCheck = true)); BasicBlock* fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst); diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 1c5f204d7ed78..33852601acebf 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -139,7 +139,7 @@ void Compiler::fgDebugCheckUpdate() /* no un-compacted blocks */ - if (fgCanCompactBlocks(block, block->Next())) + if (fgCanCompactBlocks(block)) { noway_assert(!"Found un-compacted blocks!"); } diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index e0df5c7e27b4f..deabf44974a89 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -895,12 +895,23 @@ PhaseStatus Compiler::fgPostImportationCleanup() // Returns: // true if compaction is allowed // -bool Compiler::fgCanCompactBlocks(BasicBlock* block, BasicBlock* bNext) +bool Compiler::fgCanCompactBlocks(BasicBlock* block) { assert(block != nullptr); - assert(block->NextIs(bNext)); - if (!block->KindIs(BBJ_ALWAYS) || !block->TargetIs(bNext) || block->HasFlag(BBF_KEEP_BBJ_ALWAYS)) + if (!block->KindIs(BBJ_ALWAYS) || block->HasFlag(BBF_KEEP_BBJ_ALWAYS)) + { + return false; + } + + BasicBlock* const bNext = block->GetTarget(); + + if (block == bNext) + { + return false; + } + + if (bNext->IsFirst() || (bNext == fgEntryBB) || (bNext == fgOSREntryBB)) { return false; } @@ -968,18 +979,17 @@ bool Compiler::fgCanCompactBlocks(BasicBlock* block, BasicBlock* bNext) // doDebugCheck - in Debug builds, check flowgraph for correctness after compaction // (some callers might compact blocks during destructive flowgraph changes, and thus should skip checks) // -void Compiler::fgCompactBlocks(BasicBlock* block, BasicBlock* bNext DEBUGARG(bool doDebugCheck /* = true */)) +void Compiler::fgCompactBlocks(BasicBlock* block DEBUGARG(bool doDebugCheck /* = true */)) { noway_assert(block != nullptr); + BasicBlock* const bNext = block->GetTarget(); + noway_assert(bNext != nullptr); noway_assert(!block->HasFlag(BBF_REMOVED)); noway_assert(!bNext->HasFlag(BBF_REMOVED)); - noway_assert(block->NextIs(bNext)); noway_assert(bNext->countOfInEdges() == 1 || block->isEmpty()); noway_assert(bNext->bbPreds != nullptr); - assert(block->KindIs(BBJ_ALWAYS)); - assert(block->TargetIs(bNext)); assert(!fgInDifferentRegions(block, bNext)); // Make sure the second block is not the start of a TRY block or an exception handler @@ -3287,9 +3297,9 @@ bool Compiler::fgExpandRarelyRunBlocks() } /* COMPACT blocks if possible */ - if (fgCanCompactBlocks(bPrev, block)) + if (fgCanCompactBlocks(bPrev)) { - fgCompactBlocks(bPrev, block); + fgCompactBlocks(bPrev); block = bPrev; continue; @@ -4554,11 +4564,11 @@ void Compiler::fgMoveBackwardJumpsToSuccessors() // for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB;) { - if (fgCanCompactBlocks(block, block->Next())) + if (fgCanCompactBlocks(block)) { // We haven't fixed EH information yet, so don't do any correctness checks here // - fgCompactBlocks(block, block->Next() DEBUGARG(/* doDebugCheck */ false)); + fgCompactBlocks(block DEBUGARG(/* doDebugCheck */ false)); } else { @@ -5542,13 +5552,14 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh /* COMPACT blocks if possible */ - if (fgCanCompactBlocks(block, bNext)) + if (fgCanCompactBlocks(block)) { - fgCompactBlocks(block, bNext); + fgCompactBlocks(block); /* we compacted two blocks - goto REPEAT to catch similar cases */ change = true; modified = true; + bPrev = block->Prev(); goto REPEAT; } diff --git a/src/coreclr/jit/helperexpansion.cpp b/src/coreclr/jit/helperexpansion.cpp index e62660aaab3af..07b9da25d1f60 100644 --- a/src/coreclr/jit/helperexpansion.cpp +++ b/src/coreclr/jit/helperexpansion.cpp @@ -1533,9 +1533,9 @@ bool Compiler::fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, G assert(BasicBlock::sameEHRegion(prevBb, isInitedBb)); // Extra step: merge prevBb with isInitedBb if possible - if (fgCanCompactBlocks(prevBb, isInitedBb)) + if (fgCanCompactBlocks(prevBb)) { - fgCompactBlocks(prevBb, isInitedBb); + fgCompactBlocks(prevBb); } // Clear gtInitClsHnd as a mark that we've already visited this call @@ -1862,9 +1862,9 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock, assert(BasicBlock::sameEHRegion(prevBb, fastpathBb)); // Extra step: merge prevBb with lengthCheckBb if possible - if (fgCanCompactBlocks(prevBb, lengthCheckBb)) + if (fgCanCompactBlocks(prevBb)) { - fgCompactBlocks(prevBb, lengthCheckBb); + fgCompactBlocks(prevBb); } JITDUMP("ReadUtf8: succesfully expanded!\n") @@ -2686,9 +2686,9 @@ bool Compiler::fgLateCastExpansionForCall(BasicBlock** pBlock, Statement* stmt, } // Bonus step: merge prevBb with nullcheckBb as they are likely to be mergeable - if (fgCanCompactBlocks(firstBb, nullcheckBb)) + if (fgCanCompactBlocks(firstBb)) { - fgCompactBlocks(firstBb, nullcheckBb); + fgCompactBlocks(firstBb); } return true; diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index f915cf7c34948..82e3575bb9699 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1062,9 +1062,9 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() m_b2->CopyFlags(m_b1, BBF_COPY_PROPAGATE); // Join the two blocks. This is done now to ensure that additional conditions can be chained. - if (m_b1->NextIs(m_b2) && m_comp->fgCanCompactBlocks(m_b1, m_b2)) + if (m_comp->fgCanCompactBlocks(m_b1)) { - m_comp->fgCompactBlocks(m_b1, m_b2); + m_comp->fgCompactBlocks(m_b1); } #ifdef DEBUG From 5e02f85630d15d3ff0ddb7205880656ef48d16b9 Mon Sep 17 00:00:00 2001 From: Aman Khalid Date: Wed, 19 Jun 2024 17:57:04 -0400 Subject: [PATCH 3/7] Rename --- src/coreclr/jit/compiler.h | 4 +- src/coreclr/jit/fgdiagnostic.cpp | 2 +- src/coreclr/jit/fgopt.cpp | 292 +++++++++++++--------------- src/coreclr/jit/helperexpansion.cpp | 12 +- src/coreclr/jit/optimizebools.cpp | 4 +- 5 files changed, 141 insertions(+), 173 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 486cc6077df15..80fe980a78758 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6086,9 +6086,9 @@ class Compiler void fgPrepareCallFinallyRetForRemoval(BasicBlock* block); - bool fgCanCompactBlocks(BasicBlock* block); + bool fgCanCompactBlock(BasicBlock* block); - void fgCompactBlocks(BasicBlock* block); + void fgCompactBlock(BasicBlock* block); BasicBlock* fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst); diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 33852601acebf..48c7d685ed32a 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -139,7 +139,7 @@ void Compiler::fgDebugCheckUpdate() /* no un-compacted blocks */ - if (fgCanCompactBlocks(block)) + if (fgCanCompactBlock(block)) { noway_assert(!"Found un-compacted blocks!"); } diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index a7d91338b3d13..3fe1a99d1e66a 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -886,16 +886,15 @@ PhaseStatus Compiler::fgPostImportationCleanup() } //------------------------------------------------------------- -// fgCanCompactBlocks: Determine if a block and its bbNext successor can be compacted. +// fgCanCompactBlock: Determine if a BBJ_ALWAYS block and its target can be compacted. // // Arguments: -// block - block to check. If nullptr, return false. -// bNext - bbNext of `block`. If nullptr, return false. +// block - BBJ_ALWAYS block to check // // Returns: // true if compaction is allowed // -bool Compiler::fgCanCompactBlocks(BasicBlock* block) +bool Compiler::fgCanCompactBlock(BasicBlock* block) { assert(block != nullptr); @@ -904,27 +903,27 @@ bool Compiler::fgCanCompactBlocks(BasicBlock* block) return false; } - BasicBlock* const bNext = block->GetTarget(); + BasicBlock* const target = block->GetTarget(); - if (block == bNext) + if (block == target) { return false; } - if (bNext->IsFirst() || (bNext == fgEntryBB) || (bNext == fgOSREntryBB)) + if (target->IsFirst() || (target == fgEntryBB) || (target == fgOSREntryBB)) { return false; } - // If the next block has multiple incoming edges, we can still compact if the first block is empty. + // If target has multiple incoming edges, we can still compact if block is empty. // However, not if it is the beginning of a handler. - if (bNext->countOfInEdges() != 1 && + if (target->countOfInEdges() != 1 && (!block->isEmpty() || block->HasFlag(BBF_FUNCLET_BEG) || (block->bbCatchTyp != BBCT_NONE))) { return false; } - if (bNext->HasFlag(BBF_DONT_REMOVE)) + if (target->HasFlag(BBF_DONT_REMOVE)) { return false; } @@ -935,26 +934,23 @@ bool Compiler::fgCanCompactBlocks(BasicBlock* block) return false; } - // We don't want to compact blocks that are in different Hot/Cold regions + // We don't want to compact blocks that are in different hot/cold regions // - if (fgInDifferentRegions(block, bNext)) + if (fgInDifferentRegions(block, target)) { return false; } // We cannot compact two blocks in different EH regions. // - if (fgCanRelocateEHRegions) + if (!BasicBlock::sameEHRegion(block, target)) { - if (!BasicBlock::sameEHRegion(block, bNext)) - { - return false; - } + return false; } // If there is a switch predecessor don't bother because we'd have to update the uniquesuccs as well // (if they are valid). - for (BasicBlock* const predBlock : bNext->PredBlocks()) + for (BasicBlock* const predBlock : target->PredBlocks()) { if (predBlock->KindIs(BBJ_SWITCH)) { @@ -966,168 +962,140 @@ bool Compiler::fgCanCompactBlocks(BasicBlock* block) } //------------------------------------------------------------- -// fgCompactBlocks: Compact two blocks into one. +// fgCompactBlock: Compact BBJ_ALWAYS block and its target into one. // -// Assumes that all necessary checks have been performed, i.e. fgCanCompactBlocks returns true. +// Requires that all necessary checks have been performed, i.e. fgCanCompactBlock returns true. // // Uses for this function - whenever we change links, insert blocks, ... // It will keep the flowgraph data in synch - bbNum, bbRefs, bbPreds // // Arguments: -// block - move all code into this block. -// bNext - bbNext of `block`. This block will be removed. +// block - move all code into this block from its target. // -void Compiler::fgCompactBlocks(BasicBlock* block) +void Compiler::fgCompactBlock(BasicBlock* block) { - noway_assert(block != nullptr); - BasicBlock* const bNext = block->GetTarget(); + BasicBlock* const target = block->GetTarget(); - noway_assert(bNext != nullptr); + noway_assert(target != nullptr); noway_assert(!block->HasFlag(BBF_REMOVED)); - noway_assert(!bNext->HasFlag(BBF_REMOVED)); - noway_assert(bNext->countOfInEdges() == 1 || block->isEmpty()); - noway_assert(bNext->bbPreds != nullptr); + noway_assert(!target->HasFlag(BBF_REMOVED)); + noway_assert(target->countOfInEdges() == 1 || block->isEmpty()); + noway_assert(target->bbPreds != nullptr); - assert(!fgInDifferentRegions(block, bNext)); + assert(!fgInDifferentRegions(block, target)); - // Make sure the second block is not the start of a TRY block or an exception handler + // Make sure target is not the start of a TRY block or an exception handler - noway_assert(!bbIsTryBeg(bNext)); - noway_assert(bNext->bbCatchTyp == BBCT_NONE); - noway_assert(!bNext->HasFlag(BBF_DONT_REMOVE)); + noway_assert(!bbIsTryBeg(target)); + noway_assert(target->bbCatchTyp == BBCT_NONE); + noway_assert(!target->HasFlag(BBF_DONT_REMOVE)); /* both or none must have an exception handler */ - noway_assert(block->hasTryIndex() == bNext->hasTryIndex()); + noway_assert(block->hasTryIndex() == target->hasTryIndex()); - JITDUMP("\nCompacting " FMT_BB " into " FMT_BB ":\n", bNext->bbNum, block->bbNum); + JITDUMP("\nCompacting " FMT_BB " into " FMT_BB ":\n", target->bbNum, block->bbNum); fgRemoveRefPred(block->GetTargetEdge()); - if (bNext->countOfInEdges() > 0) + if (target->countOfInEdges() > 0) { - JITDUMP("Second block has %u other incoming edges\n", bNext->countOfInEdges()); + JITDUMP("Second block has %u other incoming edges\n", target->countOfInEdges()); assert(block->isEmpty()); - // Retarget all the other edges incident on bNext - for (BasicBlock* const predBlock : bNext->PredBlocksEditing()) + // Retarget all the other edges incident on target + for (BasicBlock* const predBlock : target->PredBlocksEditing()) { - fgReplaceJumpTarget(predBlock, bNext, block); + fgReplaceJumpTarget(predBlock, target, block); } } - assert(bNext->countOfInEdges() == 0); - assert(bNext->bbPreds == nullptr); + assert(target->countOfInEdges() == 0); + assert(target->bbPreds == nullptr); /* Start compacting - move all the statements in the second block to the first block */ // First move any phi definitions of the second block after the phi defs of the first. - // TODO-CQ: This may be the wrong thing to do. If we're compacting blocks, it's because a - // control-flow choice was constant-folded away. So probably phi's need to go away, - // as well, in favor of one of the incoming branches. Or at least be modified. + // TODO-CQ: This may be the wrong thing to do. If we're compacting blocks, it's because a + // control-flow choice was constant-folded away. So probably phi's need to go away, + // as well, in favor of one of the incoming branches. Or at least be modified. - assert(block->IsLIR() == bNext->IsLIR()); + assert(block->IsLIR() == target->IsLIR()); if (block->IsLIR()) { LIR::Range& blockRange = LIR::AsRange(block); - LIR::Range& nextRange = LIR::AsRange(bNext); + LIR::Range& targetRange = LIR::AsRange(target); - // Does the next block have any phis? - GenTree* nextNode = nextRange.FirstNode(); + // Does target have any phis? + GenTree* targetNode = targetRange.FirstNode(); // Does the block have any code? - if (nextNode != nullptr) + if (targetNode != nullptr) { - LIR::Range nextNodes = nextRange.Remove(nextNode, nextRange.LastNode()); - blockRange.InsertAtEnd(std::move(nextNodes)); + LIR::Range targetNodes = targetRange.Remove(targetNode, targetRange.LastNode()); + blockRange.InsertAtEnd(std::move(targetNodes)); } } else { - Statement* blkNonPhi1 = block->FirstNonPhiDef(); - Statement* bNextNonPhi1 = bNext->FirstNonPhiDef(); - Statement* blkFirst = block->firstStmt(); - Statement* bNextFirst = bNext->firstStmt(); + Statement* blkNonPhi1 = block->FirstNonPhiDef(); + Statement* targetNonPhi1 = target->FirstNonPhiDef(); + Statement* blkFirst = block->firstStmt(); + Statement* targetFirst = target->firstStmt(); // Does the second have any phis? - if (bNextFirst != nullptr && bNextFirst != bNextNonPhi1) + if ((targetFirst != nullptr) && (targetFirst != targetNonPhi1)) { - Statement* bNextLast = bNextFirst->GetPrevStmt(); - assert(bNextLast->GetNextStmt() == nullptr); + Statement* targetLast = targetFirst->GetPrevStmt(); + assert(targetLast->GetNextStmt() == nullptr); // Does "blk" have phis? if (blkNonPhi1 != blkFirst) { // Yes, has phis. // Insert after the last phi of "block." - // First, bNextPhis after last phi of block. - Statement* blkLastPhi; - if (blkNonPhi1 != nullptr) - { - blkLastPhi = blkNonPhi1->GetPrevStmt(); - } - else - { - blkLastPhi = blkFirst->GetPrevStmt(); - } + // First, targetPhis after last phi of block. + Statement* blkLastPhi = (blkNonPhi1 != nullptr) ? blkNonPhi1->GetPrevStmt() : blkFirst->GetPrevStmt(); + blkLastPhi->SetNextStmt(targetFirst); + targetFirst->SetPrevStmt(blkLastPhi); - blkLastPhi->SetNextStmt(bNextFirst); - bNextFirst->SetPrevStmt(blkLastPhi); + // Now, rest of "block" after last phi of "target". + Statement* targetLastPhi = (targetNonPhi1 != nullptr) ? targetNonPhi1->GetPrevStmt() : targetFirst->GetPrevStmt(); + targetlastPhi->SetNextStmt(blkNonPhi1); - // Now, rest of "block" after last phi of "bNext". - Statement* bNextLastPhi = nullptr; - if (bNextNonPhi1 != nullptr) - { - bNextLastPhi = bNextNonPhi1->GetPrevStmt(); - } - else - { - bNextLastPhi = bNextFirst->GetPrevStmt(); - } - - bNextLastPhi->SetNextStmt(blkNonPhi1); if (blkNonPhi1 != nullptr) { - blkNonPhi1->SetPrevStmt(bNextLastPhi); + blkNonPhi1->SetPrevStmt(targetLastPhi); } else { // block has no non phis, so make the last statement be the last added phi. - blkFirst->SetPrevStmt(bNextLastPhi); + blkFirst->SetPrevStmt(targetLastPhi); } - // Now update the bbStmtList of "bNext". - bNext->bbStmtList = bNextNonPhi1; - if (bNextNonPhi1 != nullptr) + // Now update the bbStmtList of "target". + target->bbStmtList = targetNonPhi1; + if (targetNonPhi1 != nullptr) { - bNextNonPhi1->SetPrevStmt(bNextLast); + targetNonPhi1->SetPrevStmt(targetLast); } } else { if (blkFirst != nullptr) // If "block" has no statements, fusion will work fine... { - // First, bNextPhis at start of block. + // First, targetPhis at start of block. Statement* blkLast = blkFirst->GetPrevStmt(); - block->bbStmtList = bNextFirst; - // Now, rest of "block" (if it exists) after last phi of "bNext". - Statement* bNextLastPhi = nullptr; - if (bNextNonPhi1 != nullptr) - { - // There is a first non phi, so the last phi is before it. - bNextLastPhi = bNextNonPhi1->GetPrevStmt(); - } - else - { - // All the statements are phi defns, so the last one is the prev of the first. - bNextLastPhi = bNextFirst->GetPrevStmt(); - } - bNextFirst->SetPrevStmt(blkLast); - bNextLastPhi->SetNextStmt(blkFirst); - blkFirst->SetPrevStmt(bNextLastPhi); - // Now update the bbStmtList of "bNext" - bNext->bbStmtList = bNextNonPhi1; - if (bNextNonPhi1 != nullptr) + block->bbStmtList = targetFirst; + // Now, rest of "block" (if it exists) after last phi of "target". + Statement* targetLastPhi = (targetNonPhi1 != nullptr) ? targetNonPhi1->GetPrevStmt() : targetFirst->GetPrevStmt(); + + targetFirst->SetPrevStmt(blkLast); + targetLastPhi->SetNextStmt(blkFirst); + blkFirst->SetPrevStmt(targetLastPhi); + // Now update the bbStmtList of "target" + target->bbStmtList = targetNonPhi1; + if (targetNonPhi1 != nullptr) { - bNextNonPhi1->SetPrevStmt(bNextLast); + targetNonPhi1->SetPrevStmt(targetLast); } } } @@ -1135,7 +1103,7 @@ void Compiler::fgCompactBlocks(BasicBlock* block) // Now proceed with the updated bbTreeLists. Statement* stmtList1 = block->firstStmt(); - Statement* stmtList2 = bNext->firstStmt(); + Statement* stmtList2 = target->firstStmt(); /* the block may have an empty list */ @@ -1146,7 +1114,7 @@ void Compiler::fgCompactBlocks(BasicBlock* block) /* The second block may be a GOTO statement or something with an empty bbStmtList */ if (stmtList2 != nullptr) { - Statement* stmtLast2 = bNext->lastStmt(); + Statement* stmtLast2 = target->lastStmt(); /* append list2 to list 1 */ @@ -1157,29 +1125,29 @@ void Compiler::fgCompactBlocks(BasicBlock* block) } else { - /* block was formerly empty and now has bNext's statements */ + /* block was formerly empty and now has target's statements */ block->bbStmtList = stmtList2; } } - // If bNext is BBJ_THROW, block will become run rarely. + // If target is BBJ_THROW, block will become run rarely. // - // Otherwise, if either block or bNext has a profile weight - // or if both block and bNext have non-zero weights + // Otherwise, if either block or target has a profile weight + // or if both block and target have non-zero weights // then we will use the max weight for the block. // - if (bNext->KindIs(BBJ_THROW)) + if (target->KindIs(BBJ_THROW)) { block->bbSetRunRarely(); } else { - const bool hasProfileWeight = block->hasProfileWeight() || bNext->hasProfileWeight(); - const bool hasNonZeroWeight = (block->bbWeight > BB_ZERO_WEIGHT) || (bNext->bbWeight > BB_ZERO_WEIGHT); + const bool hasProfileWeight = block->hasProfileWeight() || target->hasProfileWeight(); + const bool hasNonZeroWeight = (block->bbWeight > BB_ZERO_WEIGHT) || (target->bbWeight > BB_ZERO_WEIGHT); if (hasProfileWeight || hasNonZeroWeight) { - weight_t const newWeight = max(block->bbWeight, bNext->bbWeight); + weight_t const newWeight = max(block->bbWeight, target->bbWeight); if (hasProfileWeight) { @@ -1195,12 +1163,12 @@ void Compiler::fgCompactBlocks(BasicBlock* block) // otherwise if either block has a zero weight we select the zero weight else { - noway_assert((block->bbWeight == BB_ZERO_WEIGHT) || (bNext->bbWeight == BB_ZERO_WEIGHT)); + noway_assert((block->bbWeight == BB_ZERO_WEIGHT) || (target->bbWeight == BB_ZERO_WEIGHT)); block->bbSetRunRarely(); } } - VarSetOps::AssignAllowUninitRhs(this, block->bbLiveOut, bNext->bbLiveOut); + VarSetOps::AssignAllowUninitRhs(this, block->bbLiveOut, target->bbLiveOut); // Update the beginning and ending IL offsets (bbCodeOffs and bbCodeOffsEnd). // Set the beginning IL offset to the minimum, and the ending offset to the maximum, of the respective blocks. @@ -1210,62 +1178,62 @@ void Compiler::fgCompactBlocks(BasicBlock* block) if (block->bbCodeOffs == BAD_IL_OFFSET) { - block->bbCodeOffs = bNext->bbCodeOffs; // If they are both BAD_IL_OFFSET, this doesn't change anything. + block->bbCodeOffs = target->bbCodeOffs; // If they are both BAD_IL_OFFSET, this doesn't change anything. } - else if (bNext->bbCodeOffs != BAD_IL_OFFSET) + else if (target->bbCodeOffs != BAD_IL_OFFSET) { // The are both valid offsets; compare them. - if (block->bbCodeOffs > bNext->bbCodeOffs) + if (block->bbCodeOffs > target->bbCodeOffs) { - block->bbCodeOffs = bNext->bbCodeOffs; + block->bbCodeOffs = target->bbCodeOffs; } } if (block->bbCodeOffsEnd == BAD_IL_OFFSET) { - block->bbCodeOffsEnd = bNext->bbCodeOffsEnd; // If they are both BAD_IL_OFFSET, this doesn't change anything. + block->bbCodeOffsEnd = target->bbCodeOffsEnd; // If they are both BAD_IL_OFFSET, this doesn't change anything. } - else if (bNext->bbCodeOffsEnd != BAD_IL_OFFSET) + else if (target->bbCodeOffsEnd != BAD_IL_OFFSET) { // The are both valid offsets; compare them. - if (block->bbCodeOffsEnd < bNext->bbCodeOffsEnd) + if (block->bbCodeOffsEnd < target->bbCodeOffsEnd) { - block->bbCodeOffsEnd = bNext->bbCodeOffsEnd; + block->bbCodeOffsEnd = target->bbCodeOffsEnd; } } - if (block->HasFlag(BBF_INTERNAL) && !bNext->HasFlag(BBF_INTERNAL)) + if (block->HasFlag(BBF_INTERNAL) && !target->HasFlag(BBF_INTERNAL)) { - // If 'block' is an internal block and 'bNext' isn't, then adjust the flags set on 'block'. + // If 'block' is an internal block and 'target' isn't, then adjust the flags set on 'block'. block->RemoveFlags(BBF_INTERNAL); // Clear the BBF_INTERNAL flag block->SetFlags(BBF_IMPORTED); // Set the BBF_IMPORTED flag } - /* Update the flags for block with those found in bNext */ + /* Update the flags for block with those found in target */ - block->CopyFlags(bNext, BBF_COMPACT_UPD); + block->CopyFlags(target, BBF_COMPACT_UPD); - /* mark bNext as removed */ + /* mark target as removed */ - bNext->SetFlags(BBF_REMOVED); + target->SetFlags(BBF_REMOVED); - /* Unlink bNext and update all the marker pointers if necessary */ + /* Unlink target and update all the marker pointers if necessary */ - fgUnlinkRange(bNext, bNext); + fgUnlinkRange(target, target); fgBBcount--; - // If bNext was the last block of a try or handler, update the EH table. + // If target was the last block of a try or handler, update the EH table. - ehUpdateForDeletedBlock(bNext); + ehUpdateForDeletedBlock(target); /* Set the jump targets */ - switch (bNext->GetKind()) + switch (target->GetKind()) { case BBJ_CALLFINALLY: // Propagate RETLESS property - block->CopyFlags(bNext, BBF_RETLESS_CALL); + block->CopyFlags(target, BBF_RETLESS_CALL); FALLTHROUGH; @@ -1273,22 +1241,22 @@ void Compiler::fgCompactBlocks(BasicBlock* block) case BBJ_EHCATCHRET: case BBJ_EHFILTERRET: { - /* Update the predecessor list for bNext's target */ - FlowEdge* const targetEdge = bNext->GetTargetEdge(); + /* Update the predecessor list for target's target */ + FlowEdge* const targetEdge = target->GetTargetEdge(); fgReplacePred(targetEdge, block); - block->SetKindAndTargetEdge(bNext->GetKind(), targetEdge); + block->SetKindAndTargetEdge(target->GetKind(), targetEdge); break; } case BBJ_COND: { - /* Update the predecessor list for bNext's true target */ - FlowEdge* const trueEdge = bNext->GetTrueEdge(); - FlowEdge* const falseEdge = bNext->GetFalseEdge(); + /* Update the predecessor list for target's true target */ + FlowEdge* const trueEdge = target->GetTrueEdge(); + FlowEdge* const falseEdge = target->GetFalseEdge(); fgReplacePred(trueEdge, block); - /* Update the predecessor list for bNext's false target if it is different from the true target */ + /* Update the predecessor list for target's false target if it is different from the true target */ if (trueEdge != falseEdge) { fgReplacePred(falseEdge, block); @@ -1299,22 +1267,22 @@ void Compiler::fgCompactBlocks(BasicBlock* block) } case BBJ_EHFINALLYRET: - block->SetEhf(bNext->GetEhfTargets()); - fgChangeEhfBlock(bNext, block); + block->SetEhf(target->GetEhfTargets()); + fgChangeEhfBlock(target, block); break; case BBJ_EHFAULTRET: case BBJ_THROW: case BBJ_RETURN: /* no jumps or fall through blocks to set here */ - block->SetKind(bNext->GetKind()); + block->SetKind(target->GetKind()); break; case BBJ_SWITCH: - block->SetSwitch(bNext->GetSwitchTargets()); - // We are moving the switch jump from bNext to block. Examine the jump targets - // of the BBJ_SWITCH at bNext and replace the predecessor to 'bNext' with ones to 'block' - fgChangeSwitchBlock(bNext, block); + block->SetSwitch(target->GetSwitchTargets()); + // We are moving the switch jump from target to block. Examine the jump targets + // of the BBJ_SWITCH at target and replace the predecessor to 'target' with ones to 'block' + fgChangeSwitchBlock(target, block); break; default: @@ -1322,7 +1290,7 @@ void Compiler::fgCompactBlocks(BasicBlock* block) break; } - assert(block->KindIs(bNext->GetKind())); + assert(block->KindIs(target->GetKind())); #if DEBUG if (verbose && 0) @@ -3293,9 +3261,9 @@ bool Compiler::fgExpandRarelyRunBlocks() } /* COMPACT blocks if possible */ - if (fgCanCompactBlocks(bPrev)) + if (fgCanCompactBlock(bPrev)) { - fgCompactBlocks(bPrev); + fgCompactBlock(bPrev); block = bPrev; continue; @@ -5616,9 +5584,9 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh /* COMPACT blocks if possible */ - if (fgCanCompactBlocks(block)) + if (fgCanCompactBlock(block)) { - fgCompactBlocks(block); + fgCompactBlock(block); /* we compacted two blocks - goto REPEAT to catch similar cases */ change = true; diff --git a/src/coreclr/jit/helperexpansion.cpp b/src/coreclr/jit/helperexpansion.cpp index 07b9da25d1f60..908d9fb129fc1 100644 --- a/src/coreclr/jit/helperexpansion.cpp +++ b/src/coreclr/jit/helperexpansion.cpp @@ -1533,9 +1533,9 @@ bool Compiler::fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, G assert(BasicBlock::sameEHRegion(prevBb, isInitedBb)); // Extra step: merge prevBb with isInitedBb if possible - if (fgCanCompactBlocks(prevBb)) + if (fgCanCompactBlock(prevBb)) { - fgCompactBlocks(prevBb); + fgCompactBlock(prevBb); } // Clear gtInitClsHnd as a mark that we've already visited this call @@ -1862,9 +1862,9 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock, assert(BasicBlock::sameEHRegion(prevBb, fastpathBb)); // Extra step: merge prevBb with lengthCheckBb if possible - if (fgCanCompactBlocks(prevBb)) + if (fgCanCompactBlock(prevBb)) { - fgCompactBlocks(prevBb); + fgCompactBlock(prevBb); } JITDUMP("ReadUtf8: succesfully expanded!\n") @@ -2686,9 +2686,9 @@ bool Compiler::fgLateCastExpansionForCall(BasicBlock** pBlock, Statement* stmt, } // Bonus step: merge prevBb with nullcheckBb as they are likely to be mergeable - if (fgCanCompactBlocks(firstBb)) + if (fgCanCompactBlock(firstBb)) { - fgCompactBlocks(firstBb); + fgCompactBlock(firstBb); } return true; diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 82e3575bb9699..9bf2875432bce 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1062,9 +1062,9 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() m_b2->CopyFlags(m_b1, BBF_COPY_PROPAGATE); // Join the two blocks. This is done now to ensure that additional conditions can be chained. - if (m_comp->fgCanCompactBlocks(m_b1)) + if (m_comp->fgCanCompactBlock(m_b1)) { - m_comp->fgCompactBlocks(m_b1); + m_comp->fgCompactBlock(m_b1); } #ifdef DEBUG From dd9bbf9cd30426b33300139929edeb221a624312 Mon Sep 17 00:00:00 2001 From: Aman Khalid Date: Wed, 19 Jun 2024 17:58:11 -0400 Subject: [PATCH 4/7] Clean up asserts --- src/coreclr/jit/fgopt.cpp | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 3fe1a99d1e66a..a03aef03289a3 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -974,25 +974,9 @@ bool Compiler::fgCanCompactBlock(BasicBlock* block) // void Compiler::fgCompactBlock(BasicBlock* block) { + assert(fgCanCompactBlock(block)); BasicBlock* const target = block->GetTarget(); - noway_assert(target != nullptr); - noway_assert(!block->HasFlag(BBF_REMOVED)); - noway_assert(!target->HasFlag(BBF_REMOVED)); - noway_assert(target->countOfInEdges() == 1 || block->isEmpty()); - noway_assert(target->bbPreds != nullptr); - - assert(!fgInDifferentRegions(block, target)); - - // Make sure target is not the start of a TRY block or an exception handler - - noway_assert(!bbIsTryBeg(target)); - noway_assert(target->bbCatchTyp == BBCT_NONE); - noway_assert(!target->HasFlag(BBF_DONT_REMOVE)); - - /* both or none must have an exception handler */ - noway_assert(block->hasTryIndex() == target->hasTryIndex()); - JITDUMP("\nCompacting " FMT_BB " into " FMT_BB ":\n", target->bbNum, block->bbNum); fgRemoveRefPred(block->GetTargetEdge()); From fe95cba5a33f85709298ae0886253bd533b03875 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 20 Jun 2024 14:45:01 -0400 Subject: [PATCH 5/7] Fix profile data transfer --- src/coreclr/jit/fgopt.cpp | 57 +++++++++++---------------------------- 1 file changed, 16 insertions(+), 41 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index a03aef03289a3..a56af92f03cb9 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1005,8 +1005,8 @@ void Compiler::fgCompactBlock(BasicBlock* block) assert(block->IsLIR() == target->IsLIR()); if (block->IsLIR()) { - LIR::Range& blockRange = LIR::AsRange(block); - LIR::Range& targetRange = LIR::AsRange(target); + LIR::Range& blockRange = LIR::AsRange(block); + LIR::Range& targetRange = LIR::AsRange(target); // Does target have any phis? GenTree* targetNode = targetRange.FirstNode(); @@ -1042,8 +1042,9 @@ void Compiler::fgCompactBlock(BasicBlock* block) targetFirst->SetPrevStmt(blkLastPhi); // Now, rest of "block" after last phi of "target". - Statement* targetLastPhi = (targetNonPhi1 != nullptr) ? targetNonPhi1->GetPrevStmt() : targetFirst->GetPrevStmt(); - targetlastPhi->SetNextStmt(blkNonPhi1); + Statement* targetLastPhi = + (targetNonPhi1 != nullptr) ? targetNonPhi1->GetPrevStmt() : targetFirst->GetPrevStmt(); + targetLastPhi->SetNextStmt(blkNonPhi1); if (blkNonPhi1 != nullptr) { @@ -1070,7 +1071,8 @@ void Compiler::fgCompactBlock(BasicBlock* block) Statement* blkLast = blkFirst->GetPrevStmt(); block->bbStmtList = targetFirst; // Now, rest of "block" (if it exists) after last phi of "target". - Statement* targetLastPhi = (targetNonPhi1 != nullptr) ? targetNonPhi1->GetPrevStmt() : targetFirst->GetPrevStmt(); + Statement* targetLastPhi = + (targetNonPhi1 != nullptr) ? targetNonPhi1->GetPrevStmt() : targetFirst->GetPrevStmt(); targetFirst->SetPrevStmt(blkLast); targetLastPhi->SetNextStmt(blkFirst); @@ -1114,42 +1116,15 @@ void Compiler::fgCompactBlock(BasicBlock* block) } } - // If target is BBJ_THROW, block will become run rarely. - // - // Otherwise, if either block or target has a profile weight - // or if both block and target have non-zero weights - // then we will use the max weight for the block. - // - if (target->KindIs(BBJ_THROW)) - { - block->bbSetRunRarely(); - } - else - { - const bool hasProfileWeight = block->hasProfileWeight() || target->hasProfileWeight(); - const bool hasNonZeroWeight = (block->bbWeight > BB_ZERO_WEIGHT) || (target->bbWeight > BB_ZERO_WEIGHT); - - if (hasProfileWeight || hasNonZeroWeight) - { - weight_t const newWeight = max(block->bbWeight, target->bbWeight); + // Transfer target's weight to block + // (target's weight should include block's weight, + // plus the weights of target's preds, which now flow into block) + const bool hasProfileWeight = block->hasProfileWeight(); + block->inheritWeight(target); - if (hasProfileWeight) - { - block->setBBProfileWeight(newWeight); - } - else - { - assert(newWeight != BB_ZERO_WEIGHT); - block->bbWeight = newWeight; - block->RemoveFlags(BBF_RUN_RARELY); - } - } - // otherwise if either block has a zero weight we select the zero weight - else - { - noway_assert((block->bbWeight == BB_ZERO_WEIGHT) || (target->bbWeight == BB_ZERO_WEIGHT)); - block->bbSetRunRarely(); - } + if (hasProfileWeight) + { + block->SetFlags(BBF_PROF_WEIGHT); } VarSetOps::AssignAllowUninitRhs(this, block->bbLiveOut, target->bbLiveOut); @@ -5575,7 +5550,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh /* we compacted two blocks - goto REPEAT to catch similar cases */ change = true; modified = true; - bPrev = block->Prev(); + bPrev = block->Prev(); goto REPEAT; } From 483e4734e2c092eeadec6acbccdd111d23d0a6a7 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 20 Jun 2024 17:12:30 -0400 Subject: [PATCH 6/7] Don't churn flowgraph with compaction after layout --- src/coreclr/jit/block.h | 2 +- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgopt.cpp | 8 ++++++-- src/coreclr/jit/lower.cpp | 4 +++- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 054edae23113b..1c4674c83826a 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -468,7 +468,7 @@ enum BasicBlockFlags : uint64_t // Flags to update when two blocks are compacted BBF_COMPACT_UPD = BBF_GC_SAFE_POINT | BBF_NEEDS_GCPOLL | BBF_HAS_JMP | BBF_HAS_IDX_LEN | BBF_HAS_MD_IDX_LEN | BBF_BACKWARD_JUMP | \ - BBF_HAS_NEWOBJ | BBF_HAS_NULLCHECK | BBF_HAS_MDARRAYREF, + BBF_HAS_NEWOBJ | BBF_HAS_NULLCHECK | BBF_HAS_MDARRAYREF | BBF_LOOP_HEAD, // Flags a block should not have had before it is split. diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 80fe980a78758..048fe584e893e 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6157,7 +6157,7 @@ class Compiler bool fgIsForwardBranch(BasicBlock* bJump, BasicBlock* bDest, BasicBlock* bSrc = nullptr); - bool fgUpdateFlowGraph(bool doTailDup = false, bool isPhase = false); + bool fgUpdateFlowGraph(bool doTailDup = false, bool isPhase = false, bool doAggressiveCompaction = true); PhaseStatus fgUpdateFlowGraphPhase(); PhaseStatus fgDfsBlocksAndRemove(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index a56af92f03cb9..3c2e7251d6f4f 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5176,6 +5176,8 @@ PhaseStatus Compiler::fgUpdateFlowGraphPhase() // Arguments: // doTailDuplication - true to attempt tail duplication optimization // isPhase - true if being run as the only thing in a phase +// doAggressiveCompaction - if false, only compact blocks that jump to the next block +// to prevent modifying the flowgraph; else, compact as much as possible // // Returns: true if the flowgraph has been modified // @@ -5183,7 +5185,9 @@ PhaseStatus Compiler::fgUpdateFlowGraphPhase() // Debuggable code and Min Optimization JIT also introduces basic blocks // but we do not optimize those! // -bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPhase /* = false */) +bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, + bool isPhase /* = false */, + bool doAggressiveCompaction /* = true */) { #ifdef DEBUG if (verbose && !isPhase) @@ -5543,7 +5547,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh /* COMPACT blocks if possible */ - if (fgCanCompactBlock(block)) + if (fgCanCompactBlock(block) && (doAggressiveCompaction || block->JumpsToNext())) { fgCompactBlock(block); diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index c7790fb6199d6..7c963c7a6e889 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7551,7 +7551,9 @@ PhaseStatus Lowering::DoPhase() // local var liveness can delete code, which may create empty blocks if (comp->opts.OptimizationEnabled()) { - bool modified = comp->fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false); + // Don't churn the flowgraph with aggressive compaction since we've already run block layout + bool modified = comp->fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false, + /* doAggressiveCompaction */ false); modified |= comp->fgRemoveDeadBlocks(); if (modified) From 9250cc8f34e6df6387bbabd444d3998efa4418f5 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 20 Jun 2024 23:49:55 -0400 Subject: [PATCH 7/7] Fix compacting call-finally pairs --- src/coreclr/jit/fgopt.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 3c2e7251d6f4f..975ad0aaa546e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -915,8 +915,16 @@ bool Compiler::fgCanCompactBlock(BasicBlock* block) return false; } + // Don't bother compacting a call-finally pair if it doesn't succeed block + // + if (target->isBBCallFinallyPair() && !block->NextIs(target)) + { + return false; + } + // If target has multiple incoming edges, we can still compact if block is empty. // However, not if it is the beginning of a handler. + // if (target->countOfInEdges() != 1 && (!block->isEmpty() || block->HasFlag(BBF_FUNCLET_BEG) || (block->bbCatchTyp != BBCT_NONE))) { @@ -929,6 +937,7 @@ bool Compiler::fgCanCompactBlock(BasicBlock* block) } // Don't compact the first block if it was specially created as a scratch block. + // if (fgBBisScratch(block)) { return false; @@ -950,6 +959,7 @@ bool Compiler::fgCanCompactBlock(BasicBlock* block) // If there is a switch predecessor don't bother because we'd have to update the uniquesuccs as well // (if they are valid). + // for (BasicBlock* const predBlock : target->PredBlocks()) { if (predBlock->KindIs(BBJ_SWITCH))