From a06198c42a48bc1d42a7c924bd34bcee00624410 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 11 Mar 2024 16:13:44 -0400 Subject: [PATCH 1/3] Add fgRedirectTrueEdge, fgRedirectFalseEdge --- src/coreclr/jit/compiler.h | 12 ++--- src/coreclr/jit/fgbasic.cpp | 41 ++++----------- src/coreclr/jit/fgehopt.cpp | 82 +---------------------------- src/coreclr/jit/fgflow.cpp | 78 +++++++++++++++++++++++++++ src/coreclr/jit/fgopt.cpp | 38 ++++++------- src/coreclr/jit/flowgraph.cpp | 6 +-- src/coreclr/jit/helperexpansion.cpp | 42 +++++++-------- src/coreclr/jit/importer.cpp | 38 ++++++++----- src/coreclr/jit/optimizebools.cpp | 4 +- src/coreclr/jit/optimizer.cpp | 8 ++- 10 files changed, 162 insertions(+), 187 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 3e789dc3a474cd..8bc8bd5360055e 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5229,14 +5229,6 @@ class Compiler void fgCleanupContinuation(BasicBlock* continuation); PhaseStatus fgTailMergeThrows(); - void fgTailMergeThrowsFallThroughHelper(BasicBlock* predBlock, - BasicBlock* nonCanonicalBlock, - BasicBlock* canonicalBlock, - FlowEdge* predEdge); - void fgTailMergeThrowsJumpToHelper(BasicBlock* predBlock, - BasicBlock* nonCanonicalBlock, - BasicBlock* canonicalBlock, - FlowEdge* predEdge); bool fgRetargetBranchesToCanonicalCallFinally(BasicBlock* block, BasicBlock* handler, @@ -5918,6 +5910,10 @@ class Compiler public: void fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget); + void fgRedirectTrueEdge(BasicBlock* block, BasicBlock* newTarget); + + void fgRedirectFalseEdge(BasicBlock* block, BasicBlock* newTarget); + void fgFindBasicBlocks(); bool fgCheckEHCanInsertAfterBlock(BasicBlock* blk, unsigned regionIndex, bool putInTryRegion); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 98c57006dae295..692d620cceb8b7 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -652,49 +652,30 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas case BBJ_COND: if (block->TrueTargetIs(oldTarget)) { - FlowEdge* const oldEdge = block->GetTrueEdge(); - - if (block->FalseEdgeIs(oldEdge)) + if (block->FalseEdgeIs(block->GetTrueEdge())) { - // fgRemoveRefPred returns nullptr for BBJ_COND blocks with two flow edges to target fgRemoveConditionalJump(block); assert(block->KindIs(BBJ_ALWAYS)); assert(block->TargetIs(oldTarget)); - } - - // fgRemoveRefPred should have removed the flow edge - fgRemoveRefPred(oldEdge); - assert(oldEdge->getDupCount() == 0); - - // TODO-NoFallThrough: Proliferate weight from oldEdge - // (as a quirk, we avoid doing so for the true target to reduce diffs for now) - FlowEdge* const newEdge = fgAddRefPred(newTarget, block); - - if (block->KindIs(BBJ_ALWAYS)) - { - block->SetTargetEdge(newEdge); + fgRedirectTargetEdge(block, newTarget); } else { - assert(block->KindIs(BBJ_COND)); - block->SetTrueEdge(newEdge); - - if (oldEdge->hasLikelihood()) - { - newEdge->setLikelihood(oldEdge->getLikelihood()); - } + fgRedirectTrueEdge(block, newTarget); } } else { assert(block->FalseTargetIs(oldTarget)); - FlowEdge* const oldEdge = block->GetFalseEdge(); + assert(!block->TrueEdgeIs(block->GetFalseEdge())); + fgRedirectFalseEdge(block, newTarget); + } - // fgRemoveRefPred should have removed the flow edge - fgRemoveRefPred(oldEdge); - assert(oldEdge->getDupCount() == 0); - FlowEdge* const newEdge = fgAddRefPred(newTarget, block, oldEdge); - block->SetFalseEdge(newEdge); + if (block->KindIs(BBJ_COND) && block->TrueEdgeIs(block->GetFalseEdge())) + { + fgRemoveConditionalJump(block); + assert(block->KindIs(BBJ_ALWAYS)); + assert(block->TargetIs(newTarget)); } break; diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index a284b318d2e57d..a437a3da128d4c 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -2006,26 +2006,8 @@ PhaseStatus Compiler::fgTailMergeThrows() { switch (predBlock->GetKind()) { - // TODO: Use fgReplaceJumpTarget once it preserves edge weights for BBJ_COND blocks - case BBJ_COND: - { - // Flow to non canonical block could be via true or false target. - if (predBlock->FalseTargetIs(nonCanonicalBlock)) - { - fgTailMergeThrowsFallThroughHelper(predBlock, nonCanonicalBlock, canonicalBlock, - predBlock->GetFalseEdge()); - } - - if (predBlock->TrueTargetIs(nonCanonicalBlock)) - { - fgTailMergeThrowsJumpToHelper(predBlock, nonCanonicalBlock, canonicalBlock, - predBlock->GetTrueEdge()); - } - updated = true; - } - break; - case BBJ_ALWAYS: + case BBJ_COND: case BBJ_SWITCH: { JITDUMP("*** " FMT_BB " now branching to " FMT_BB "\n", predBlock->bbNum, canonicalBlock->bbNum); @@ -2068,65 +2050,3 @@ PhaseStatus Compiler::fgTailMergeThrows() fgModified = false; return PhaseStatus::MODIFIED_EVERYTHING; } - -//------------------------------------------------------------------------ -// fgTailMergeThrowsFallThroughHelper: fixup flow for fall throughs to mergeable throws -// -// Arguments: -// predBlock - block falling through to the throw helper -// nonCanonicalBlock - original fall through target -// canonicalBlock - new (jump) target -// predEdge - original flow edge -// -// Notes: -// Alters fall through flow of predBlock so it jumps to the -// canonicalBlock via a new basic block. Does not try and fix -// jump-around flow; we leave that to optOptimizeFlow which runs -// just afterwards. -// -void Compiler::fgTailMergeThrowsFallThroughHelper(BasicBlock* predBlock, - BasicBlock* nonCanonicalBlock, - BasicBlock* canonicalBlock, - FlowEdge* predEdge) -{ - assert(predBlock->KindIs(BBJ_COND)); - assert(predBlock->FalseTargetIs(nonCanonicalBlock)); - - JITDUMP("*** " FMT_BB " false target is now " FMT_BB "\n", predBlock->bbNum, canonicalBlock->bbNum); - - // Remove the old flow - fgRemoveRefPred(predEdge); - - // Wire up the new flow - FlowEdge* const falseEdge = fgAddRefPred(canonicalBlock, predBlock, predEdge); - predBlock->SetFalseEdge(falseEdge); -} - -//------------------------------------------------------------------------ -// fgTailMergeThrowsJumpToHelper: fixup flow for jumps to mergeable throws -// -// Arguments: -// predBlock - block jumping to the throw helper -// nonCanonicalBlock - original jump target -// canonicalBlock - new jump target -// predEdge - original flow edge -// -// Notes: -// Alters jumpDest of predBlock so it jumps to the canonicalBlock. -// -void Compiler::fgTailMergeThrowsJumpToHelper(BasicBlock* predBlock, - BasicBlock* nonCanonicalBlock, - BasicBlock* canonicalBlock, - FlowEdge* predEdge) -{ - JITDUMP("*** " FMT_BB " now branching to " FMT_BB "\n", predBlock->bbNum, canonicalBlock->bbNum); - - // Remove the old flow - assert(predBlock->KindIs(BBJ_COND)); - assert(predBlock->TrueTargetIs(nonCanonicalBlock)); - fgRemoveRefPred(predBlock->GetTrueEdge()); - - // Wire up the new flow - FlowEdge* const trueEdge = fgAddRefPred(canonicalBlock, predBlock, predEdge); - predBlock->SetTrueEdge(trueEdge); -} diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index d77775a1cde938..23ec015e669b31 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -449,6 +449,84 @@ void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) newTarget->bbRefs++; } +void Compiler::fgRedirectTrueEdge(BasicBlock* block, BasicBlock* newTarget) +{ + assert(block != nullptr); + assert(newTarget != nullptr); + assert(block->KindIs(BBJ_COND)); + assert(!block->TrueEdgeIs(block->GetFalseEdge())); + + // Update oldTarget's pred list. + // We could call fgRemoveRefPred, but since we're removing the one and only ref from block to oldTarget, + // fgRemoveAllRefPreds is slightly more efficient (one fewer branch, doesn't update edge's dup count, etc). + // + FlowEdge* trueEdge = block->GetTrueEdge(); + BasicBlock* oldTarget = trueEdge->getDestinationBlock(); + fgRemoveAllRefPreds(oldTarget, block); + + // Splice edge into new target block's pred list + // + FlowEdge** predListPtr = fgGetPredInsertPoint(block, newTarget); + FlowEdge* predEdge = *predListPtr; + + if (block->FalseEdgeIs(predEdge)) + { + block->SetTrueEdge(predEdge); + predEdge->incrementDupCount(); + } + else + { + trueEdge->setNextPredEdge(predEdge); + trueEdge->setDestinationBlock(newTarget); + *predListPtr = trueEdge; + } + + newTarget->bbRefs++; + + // Pred list of target should (stilL) be ordered + // + assert(newTarget->checkPredListOrder()); +} + +void Compiler::fgRedirectFalseEdge(BasicBlock* block, BasicBlock* newTarget) +{ + assert(block != nullptr); + assert(newTarget != nullptr); + assert(block->KindIs(BBJ_COND)); + assert(!block->TrueEdgeIs(block->GetFalseEdge())); + + // Update oldTarget's pred list. + // We could call fgRemoveRefPred, but since we're removing the one and only ref from block to oldTarget, + // fgRemoveAllRefPreds is slightly more efficient (one fewer branch, doesn't update edge's dup count, etc). + // + FlowEdge* falseEdge = block->GetFalseEdge(); + BasicBlock* oldTarget = falseEdge->getDestinationBlock(); + fgRemoveAllRefPreds(oldTarget, block); + + // Splice edge into new target block's pred list + // + FlowEdge** predListPtr = fgGetPredInsertPoint(block, newTarget); + FlowEdge* predEdge = *predListPtr; + + if (block->TrueEdgeIs(predEdge)) + { + block->SetFalseEdge(predEdge); + predEdge->incrementDupCount(); + } + else + { + falseEdge->setNextPredEdge(predEdge); + falseEdge->setDestinationBlock(newTarget); + *predListPtr = falseEdge; + } + + newTarget->bbRefs++; + + // Pred list of target should (stilL) be ordered + // + assert(newTarget->checkPredListOrder()); +} + Compiler::SwitchUniqueSuccSet Compiler::GetDescriptorForSwitch(BasicBlock* switchBlk) { assert(switchBlk->KindIs(BBJ_SWITCH)); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 48e14ed6b04f7b..6f5c03f48e363a 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1576,16 +1576,12 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc if (block->TrueTargetIs(bDest)) { assert(!block->FalseTargetIs(bDest)); - fgRemoveRefPred(block->GetTrueEdge()); - FlowEdge* const trueEdge = fgAddRefPred(bDest->GetTarget(), block, block->GetTrueEdge()); - block->SetTrueEdge(trueEdge); + fgRedirectTrueEdge(block, bDest->GetTarget()); } else { assert(block->FalseTargetIs(bDest)); - fgRemoveRefPred(block->GetFalseEdge()); - FlowEdge* const falseEdge = fgAddRefPred(bDest->GetTarget(), block, block->GetFalseEdge()); - block->SetFalseEdge(falseEdge); + fgRedirectFalseEdge(block, bDest->GetTarget()); } break; @@ -2506,11 +2502,11 @@ bool Compiler::fgOptimizeUncondBranchToSimpleCond(BasicBlock* block, BasicBlock* // Fix up block's flow. // Assume edge likelihoods transfer over. // - fgRemoveRefPred(block->GetTargetEdge()); + fgRedirectTargetEdge(block, target->GetTrueTarget()); + block->GetTargetEdge()->setLikelihood(target->GetTrueEdge()->getLikelihood()); - FlowEdge* const trueEdge = fgAddRefPred(target->GetTrueTarget(), block, target->GetTrueEdge()); FlowEdge* const falseEdge = fgAddRefPred(target->GetFalseTarget(), block, target->GetFalseEdge()); - block->SetCond(trueEdge, falseEdge); + block->SetCond(block->GetTargetEdge(), falseEdge); JITDUMP("fgOptimizeUncondBranchToSimpleCond(from " FMT_BB " to cond " FMT_BB "), modified " FMT_BB "\n", block->bbNum, target->bbNum, block->bbNum); @@ -2939,15 +2935,12 @@ bool Compiler::fgOptimizeBranch(BasicBlock* bJump) // FlowEdge* const falseEdge = fgAddRefPred(bJump->Next(), bJump, destFalseEdge); - // bJump no longer jumps to bDest - // - fgRemoveRefPred(bJump->GetTargetEdge()); - // bJump now jumps to bDest's normal jump target // - FlowEdge* const trueEdge = fgAddRefPred(bDestNormalTarget, bJump, destTrueEdge); + fgRedirectTargetEdge(bJump, bDestNormalTarget); + bJump->GetTargetEdge()->setLikelihood(destTrueEdge->getLikelihood()); - bJump->SetCond(trueEdge, falseEdge); + bJump->SetCond(bJump->GetTargetEdge(), falseEdge); if (weightJump > 0) { @@ -5064,8 +5057,8 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh // Rewire flow from block // block->SetFalseEdge(oldTrueEdge); - FlowEdge* const newTrueEdge = fgAddRefPred(bNext->GetTarget(), block, oldFalseEdge); - block->SetTrueEdge(newTrueEdge); + block->SetTrueEdge(oldFalseEdge); + fgRedirectTrueEdge(block, bNext->GetTarget()); /* Unlink bNext from the BasicBlock list; note that we can @@ -5711,11 +5704,14 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) // if (commSucc != nullptr) { - fgRemoveRefPred(predBlock->GetTargetEdge()); + assert(predBlock->KindIs(BBJ_ALWAYS)); + fgRedirectTargetEdge(predBlock, crossJumpTarget); + } + else + { + FlowEdge* const newEdge = fgAddRefPred(crossJumpTarget, predBlock); + predBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); } - - FlowEdge* const newEdge = fgAddRefPred(crossJumpTarget, predBlock); - predBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); } // We changed things diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 7e6d0b6dc5c2f4..5068658c4843a4 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -2772,7 +2772,7 @@ void Compiler::fgInsertFuncletPrologBlock(BasicBlock* block) // the handler go to the prolog. Edges coming from with the handler are back-edges, and // go to the existing 'block'. - for (BasicBlock* const predBlock : block->PredBlocks()) + for (BasicBlock* const predBlock : block->PredBlocksEditing()) { if (!fgIsIntraHandlerPred(predBlock, block)) { @@ -2784,9 +2784,7 @@ void Compiler::fgInsertFuncletPrologBlock(BasicBlock* block) case BBJ_CALLFINALLY: { noway_assert(predBlock->TargetIs(block)); - fgRemoveRefPred(predBlock->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(newHead, predBlock); - predBlock->SetTargetEdge(newEdge); + fgRedirectTargetEdge(predBlock, newHead); break; } diff --git a/src/coreclr/jit/helperexpansion.cpp b/src/coreclr/jit/helperexpansion.cpp index b9db4a6ddc9177..3259f24f88aa07 100644 --- a/src/coreclr/jit/helperexpansion.cpp +++ b/src/coreclr/jit/helperexpansion.cpp @@ -2489,25 +2489,6 @@ bool Compiler::fgLateCastExpansionForCall(BasicBlock** pBlock, Statement* stmt, fgRedirectTargetEdge(firstBb, nullcheckBb); - { - FlowEdge* const trueEdge = fgAddRefPred(lastBb, nullcheckBb); - nullcheckBb->SetTrueEdge(trueEdge); - } - - if (typeCheckNotNeeded) - { - FlowEdge* const falseEdge = fgAddRefPred(fallbackBb, nullcheckBb); - nullcheckBb->SetFalseEdge(falseEdge); - } - else - { - FlowEdge* const falseEdge = fgAddRefPred(typeChecksBbs[0], nullcheckBb); - nullcheckBb->SetFalseEdge(falseEdge); - - FlowEdge* const newEdge = fgAddRefPred(lastBb, typeCheckSucceedBb); - typeCheckSucceedBb->SetTargetEdge(newEdge); - } - // // Re-distribute weights // @@ -2558,13 +2539,32 @@ bool Compiler::fgLateCastExpansionForCall(BasicBlock** pBlock, Statement* stmt, assert(BasicBlock::sameEHRegion(firstBb, nullcheckBb)); assert(BasicBlock::sameEHRegion(firstBb, fallbackBb)); + FlowEdge* falseEdge; + + if (typeCheckNotNeeded) + { + falseEdge = fgAddRefPred(fallbackBb, nullcheckBb); + } + else + { + falseEdge = fgAddRefPred(typeChecksBbs[0], nullcheckBb); + + FlowEdge* const newEdge = fgAddRefPred(lastBb, typeCheckSucceedBb); + typeCheckSucceedBb->SetTargetEdge(newEdge); + } + // call guarantees that obj is never null, we can drop the nullcheck // by converting it to a BBJ_ALWAYS to its false target. if ((call->gtCallMoreFlags & GTF_CALL_M_CAST_OBJ_NONNULL) != 0) { fgRemoveStmt(nullcheckBb, nullcheckBb->lastStmt()); - fgRemoveRefPred(nullcheckBb->GetTrueEdge()); - nullcheckBb->SetKindAndTargetEdge(BBJ_ALWAYS, nullcheckBb->GetFalseEdge()); + nullcheckBb->SetKindAndTargetEdge(BBJ_ALWAYS, falseEdge); + } + else + { + FlowEdge* const trueEdge = fgAddRefPred(lastBb, nullcheckBb); + nullcheckBb->SetTrueEdge(trueEdge); + nullcheckBb->SetFalseEdge(falseEdge); } // Bonus step: merge prevBb with nullcheckBb as they are likely to be mergeable diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index e50773eefd4a53..35ecb39b86b865 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4403,10 +4403,10 @@ void Compiler::impImportLeave(BasicBlock* block) assert(step == DUMMY_INIT(NULL)); callBlock = block; + // callBlock calls the finally handler assert(callBlock->HasInitializedTarget()); - fgRemoveRefPred(callBlock->GetTargetEdge()); - - // callBlock will call the finally handler. This will be set up later. + fgRedirectTargetEdge(callBlock, HBtab->ebdHndBeg); + callBlock->SetKind(BBJ_CALLFINALLY); if (endCatches) { @@ -4428,8 +4428,10 @@ void Compiler::impImportLeave(BasicBlock* block) // Calling the finally block. - // callBlock will call the finally handler. This will be set up later. - callBlock = fgNewBBinRegion(BBJ_CALLFINALLY, XTnum + 1, 0, step); + // callBlock calls the finally handler + callBlock = fgNewBBinRegion(BBJ_CALLFINALLY, XTnum + 1, 0, step); + FlowEdge* const newEdge = fgAddRefPred(HBtab->ebdHndBeg, callBlock); + callBlock->SetTargetEdge(newEdge); // step's jump target shouldn't be set yet assert(!step->HasInitializedTarget()); @@ -4466,6 +4468,9 @@ void Compiler::impImportLeave(BasicBlock* block) impEndTreeList(callBlock, endLFinStmt, lastStmt); } + // callBlock should be set up at this point + assert(callBlock->TargetIs(HBtab->ebdHndBeg)); + // Note: we don't know the jump target yet step = fgNewBBafter(BBJ_CALLFINALLYRET, callBlock, true); // The new block will inherit this block's weight. @@ -4484,9 +4489,6 @@ void Compiler::impImportLeave(BasicBlock* block) unsigned finallyNesting = compHndBBtab[XTnum].ebdHandlerNestingLevel; assert(finallyNesting <= compHndBBtabCount); - FlowEdge* const newEdge = fgAddRefPred(HBtab->ebdHndBeg, callBlock); - callBlock->SetKindAndTargetEdge(BBJ_CALLFINALLY, newEdge); - GenTree* endLFin = new (this, GT_END_LFIN) GenTreeVal(GT_END_LFIN, TYP_VOID, finallyNesting); endLFinStmt = gtNewStmt(endLFin); endCatches = NULL; @@ -4739,6 +4741,10 @@ void Compiler::impImportLeave(BasicBlock* block) callBlock->inheritWeight(block); callBlock->SetFlags(BBF_IMPORTED); + // callBlock calls the finally handler + FlowEdge* const newEdge = fgAddRefPred(HBtab->ebdHndBeg, callBlock); + callBlock->SetKindAndTargetEdge(BBJ_CALLFINALLY, newEdge); + #ifdef DEBUG if (verbose) { @@ -4752,10 +4758,10 @@ void Compiler::impImportLeave(BasicBlock* block) callBlock = block; + // callBlock calls the finally handler assert(callBlock->HasInitializedTarget()); - fgRemoveRefPred(callBlock->GetTargetEdge()); - -// callBlock will call the finally handler. This will be set up later. + fgRedirectTargetEdge(callBlock, HBtab->ebdHndBeg); + callBlock->SetKind(BBJ_CALLFINALLY); #ifdef DEBUG if (verbose) @@ -4854,6 +4860,10 @@ void Compiler::impImportLeave(BasicBlock* block) callBlock->inheritWeight(block); callBlock->SetFlags(BBF_IMPORTED); + // callBlock calls the finally handler + FlowEdge* const newEdge = fgAddRefPred(HBtab->ebdHndBeg, callBlock); + callBlock->SetKindAndTargetEdge(BBJ_CALLFINALLY, newEdge); + #ifdef DEBUG if (verbose) { @@ -4864,6 +4874,9 @@ void Compiler::impImportLeave(BasicBlock* block) #endif } + // callBlock should be set up at this point + assert(callBlock->TargetIs(HBtab->ebdHndBeg)); + // Note: we don't know the jump target yet step = fgNewBBafter(BBJ_CALLFINALLYRET, callBlock, true); stepType = ST_FinallyReturn; @@ -4881,9 +4894,6 @@ void Compiler::impImportLeave(BasicBlock* block) XTnum, step->bbNum); } #endif - - FlowEdge* const newEdge = fgAddRefPred(HBtab->ebdHndBeg, callBlock); - callBlock->SetKindAndTargetEdge(BBJ_CALLFINALLY, newEdge); } else if (HBtab->HasCatchHandler() && jitIsBetween(blkAddr, tryBeg, tryEnd) && !jitIsBetween(jmpAddr, tryBeg, tryEnd)) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 5b609c0b4fb090..d6d5160ca21e05 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1282,9 +1282,7 @@ void OptBoolsDsc::optOptimizeBoolsUpdateTrees() else { edge2 = m_b2->GetFalseEdge(); - m_comp->fgRemoveRefPred(m_b1->GetTrueEdge()); - FlowEdge* const newEdge = m_comp->fgAddRefPred(m_b2->GetTrueTarget(), m_b1); - m_b1->SetTrueEdge(newEdge); + m_comp->fgRedirectTrueEdge(m_b1, m_b2->GetTrueTarget()); } assert(edge1 != nullptr); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 41c9f73edaf4f4..30f5cbb6f3a763 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -1724,12 +1724,10 @@ void Compiler::optRedirectPrevUnrollIteration(FlowGraphNaturalLoop* loop, BasicB testCopyStmt->SetRootNode(sideEffList); } - fgRemoveRefPred(prevTestBlock->GetTrueEdge()); - fgRemoveRefPred(prevTestBlock->GetFalseEdge()); - // Redirect exit edge from previous iteration to new entry. - FlowEdge* const newEdge = fgAddRefPred(target, prevTestBlock); - prevTestBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); + fgRedirectTrueEdge(prevTestBlock, target); + fgRemoveRefPred(prevTestBlock->GetFalseEdge()); + prevTestBlock->SetKindAndTargetEdge(BBJ_ALWAYS, prevTestBlock->GetTrueEdge()); JITDUMP("Redirecting previously created exiting " FMT_BB " -> " FMT_BB "\n", prevTestBlock->bbNum, target->bbNum); From 6e444dc89f0d0af86d21d226299300e63a7ebaed Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 11 Mar 2024 16:58:44 -0400 Subject: [PATCH 2/3] Fix build --- src/coreclr/jit/importer.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 35ecb39b86b865..49bdd4e5e77576 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4429,16 +4429,21 @@ void Compiler::impImportLeave(BasicBlock* block) // Calling the finally block. // callBlock calls the finally handler - callBlock = fgNewBBinRegion(BBJ_CALLFINALLY, XTnum + 1, 0, step); - FlowEdge* const newEdge = fgAddRefPred(HBtab->ebdHndBeg, callBlock); - callBlock->SetTargetEdge(newEdge); + callBlock = fgNewBBinRegion(BBJ_CALLFINALLY, XTnum + 1, 0, step); + + { + FlowEdge* const newEdge = fgAddRefPred(HBtab->ebdHndBeg, callBlock); + callBlock->SetTargetEdge(newEdge); + } // step's jump target shouldn't be set yet assert(!step->HasInitializedTarget()); - // the previous call to a finally returns to this call (to the next finally in the chain) - FlowEdge* const newEdge = fgAddRefPred(callBlock, step); - step->SetTargetEdge(newEdge); + { + // the previous call to a finally returns to this call (to the next finally in the chain) + FlowEdge* const newEdge = fgAddRefPred(callBlock, step); + step->SetTargetEdge(newEdge); + } // The new block will inherit this block's weight. callBlock->inheritWeight(block); From b8a40c4b491f6ec09f6774449c08f335d5dff6a9 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 12 Mar 2024 13:22:44 -0400 Subject: [PATCH 3/3] Feedback --- src/coreclr/jit/fgflow.cpp | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 23ec015e669b31..f9a1721218683b 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -449,6 +449,23 @@ void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) newTarget->bbRefs++; } +//------------------------------------------------------------------------ +// fgRedirectTrueEdge: Sets block->bbTrueEdge's target block to newTarget, +// updating pred lists as necessary. +// +// Arguments: +// block -- The block we want to make a predecessor of newTarget. +// It could be one already, in which case nothing changes. +// newTarget -- The new successor of block. +// +// Notes: +// This assumes block's true and false targets are different. +// If setting block's true target to its false target, +// fgRedirectTrueEdge increments the false edge's dup count, +// and ensures block->bbTrueEdge == block->bbFalseEdge. +// We don't update newTarget->bbPreds in this case, +// as we don't want to have more than one edge from the same predecessor. +// void Compiler::fgRedirectTrueEdge(BasicBlock* block, BasicBlock* newTarget) { assert(block != nullptr); @@ -483,11 +500,28 @@ void Compiler::fgRedirectTrueEdge(BasicBlock* block, BasicBlock* newTarget) newTarget->bbRefs++; - // Pred list of target should (stilL) be ordered + // Pred list of target should (still) be ordered // assert(newTarget->checkPredListOrder()); } +//------------------------------------------------------------------------ +// fgRedirectFalseEdge: Sets block->bbFalseEdge's target block to newTarget, +// updating pred lists as necessary. +// +// Arguments: +// block -- The block we want to make a predecessor of newTarget. +// It could be one already, in which case nothing changes. +// newTarget -- The new successor of block. +// +// Notes: +// This assumes block's true and false targets are different. +// If setting block's false target to its true target, +// fgRedirectFalseEdge increments the true edge's dup count, +// and ensures block->bbTrueEdge == block->bbFalseEdge. +// We don't update newTarget->bbPreds in this case, +// as we don't want to have more than one edge from the same predecessor. +// void Compiler::fgRedirectFalseEdge(BasicBlock* block, BasicBlock* newTarget) { assert(block != nullptr); @@ -522,7 +556,7 @@ void Compiler::fgRedirectFalseEdge(BasicBlock* block, BasicBlock* newTarget) newTarget->bbRefs++; - // Pred list of target should (stilL) be ordered + // Pred list of target should (still) be ordered // assert(newTarget->checkPredListOrder()); }