From 3c4f370f1bbf8628e08a64d7a4f71b103406ad81 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 5 Mar 2024 17:44:58 -0500 Subject: [PATCH 1/7] Add fgRedirectTargetEdge --- src/coreclr/jit/compiler.h | 4 ++ src/coreclr/jit/fgbasic.cpp | 4 +- src/coreclr/jit/fgehopt.cpp | 26 +++------ src/coreclr/jit/fgflow.cpp | 45 +++++++++++++++ src/coreclr/jit/fginline.cpp | 4 +- src/coreclr/jit/fgopt.cpp | 8 +-- src/coreclr/jit/helperexpansion.cpp | 55 +++++------------- src/coreclr/jit/importer.cpp | 62 ++++++++++++--------- src/coreclr/jit/indirectcalltransformer.cpp | 4 +- src/coreclr/jit/jiteh.cpp | 4 +- src/coreclr/jit/morph.cpp | 6 +- src/coreclr/jit/optimizer.cpp | 5 +- 12 files changed, 116 insertions(+), 111 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index fefa99c750b1a..ea81bb360c61e 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5902,6 +5902,10 @@ class Compiler template FlowEdge* fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowEdge* oldEdge = nullptr); + void fgUpdateEdgeTarget(FlowEdge* edge, BasicBlock* newTarget); + + void fgRedirectTargetEdge(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 5558233924037..9a86ac91875c7 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -4192,9 +4192,7 @@ void Compiler::fgFixEntryFlowForOSR() // fgEnsureFirstBBisScratch(); assert(fgFirstBB->KindIs(BBJ_ALWAYS) && fgFirstBB->JumpsToNext()); - fgRemoveRefPred(fgFirstBB->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(fgOSREntryBB, fgFirstBB); - fgFirstBB->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); + fgRedirectTargetEdge(fgFirstBB, fgOSREntryBB); // We don't know the right weight for this block, since // execution of the method was interrupted within the diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 51c9aa9aaa1a9..6b5d286841255 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -168,10 +168,8 @@ PhaseStatus Compiler::fgRemoveEmptyFinally() fgRemoveBlock(leaveBlock, /* unreachable */ true); // Ref count updates. - fgRemoveRefPred(currentBlock->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(postTryFinallyBlock, currentBlock); - - currentBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); + fgRedirectTargetEdge(currentBlock, postTryFinallyBlock); + currentBlock->SetKind(BBJ_ALWAYS); currentBlock->RemoveFlags(BBF_RETLESS_CALL); // no longer a BBJ_CALLFINALLY // Cleanup the postTryFinallyBlock @@ -1136,12 +1134,11 @@ PhaseStatus Compiler::fgCloneFinally() fgRemoveBlock(leaveBlock, /* unreachable */ true); // Ref count updates. - fgRemoveRefPred(currentBlock->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(firstCloneBlock, currentBlock); + fgRedirectTargetEdge(currentBlock, firstCloneBlock); // This call returns to the expected spot, so retarget it to branch to the clone. currentBlock->RemoveFlags(BBF_RETLESS_CALL); // no longer a BBJ_CALLFINALLY - currentBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); + currentBlock->SetKind(BBJ_ALWAYS); // Make sure iteration isn't going off the deep end. assert(leaveBlock != endCallFinallyRangeBlock); @@ -1758,9 +1755,7 @@ bool Compiler::fgRetargetBranchesToCanonicalCallFinally(BasicBlock* block, canonicalCallFinally->bbNum); assert(callFinally->bbRefs > 0); - fgRemoveRefPred(block->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(canonicalCallFinally, block); - block->SetTargetEdge(newEdge); + fgRedirectTargetEdge(block, canonicalCallFinally); // Update profile counts // @@ -2132,16 +2127,12 @@ void Compiler::fgTailMergeThrowsJumpToHelper(BasicBlock* predBlock, { JITDUMP("*** " FMT_BB " now branching to " FMT_BB "\n", predBlock->bbNum, canonicalBlock->bbNum); - FlowEdge* const newEdge = fgAddRefPred(canonicalBlock, predBlock, predEdge); if (predBlock->KindIs(BBJ_ALWAYS)) { - // Remove the old flow + // Update flow to new target assert(predBlock->TargetIs(nonCanonicalBlock)); - fgRemoveRefPred(predBlock->GetTargetEdge()); - - // Wire up the new flow - predBlock->SetTargetEdge(newEdge); + fgRedirectTargetEdge(predBlock, canonicalBlock); } else { @@ -2151,6 +2142,7 @@ void Compiler::fgTailMergeThrowsJumpToHelper(BasicBlock* predBlock, fgRemoveRefPred(predBlock->GetTrueEdge()); // Wire up the new flow - predBlock->SetTrueEdge(newEdge); + 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 79e9d41d7dc96..87d70b72d58ce 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -380,6 +380,51 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block) } } +void Compiler::fgUpdateEdgeTarget(FlowEdge* edge, BasicBlock* newTarget) +{ + assert(edge != nullptr); + assert(newTarget != nullptr); + assert(fgPredsComputed); + + newTarget->bbRefs++; + + FlowEdge** listp = &newTarget->bbPreds; + + // References are added randomly, so we have to search. + // + BasicBlock* const blockPred = edge->getSourceBlock(); + while ((*listp != nullptr) && ((*listp)->getSourceBlock()->bbNum < blockPred->bbNum)) + { + listp = (*listp)->getNextPredEdgeRef(); + } + + // Splice new edge into the list in the correct ordered location. + // + edge->setNextPredEdge(*listp); + edge->setDestinationBlock(newTarget); + *listp = edge; + + // Pred list should (still) be ordered. + // + assert(newTarget->checkPredListOrder()); +} + +void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) +{ + assert(block != nullptr); + assert(newTarget != nullptr); + + const bool fgModifiedOld = fgModified; + FlowEdge* targetEdge = block->GetTargetEdge(); + BasicBlock* oldTarget = targetEdge->getDestinationBlock(); + + fgRemoveAllRefPreds(oldTarget, block); + fgUpdateEdgeTarget(targetEdge, newTarget); + assert(targetEdge->getLikelihood() == 1.0); + + fgModified = fgModifiedOld || (newTarget != oldTarget); +} + Compiler::SwitchUniqueSuccSet Compiler::GetDescriptorForSwitch(BasicBlock* switchBlk) { assert(switchBlk->KindIs(BBJ_SWITCH)); diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 24ce934770fea..5267cdebfd0f8 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -1550,11 +1550,9 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo) // Insert inlinee's blocks into inliner's block list. assert(topBlock->KindIs(BBJ_ALWAYS)); assert(topBlock->TargetIs(bottomBlock)); - fgRemoveRefPred(topBlock->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(InlineeCompiler->fgFirstBB, topBlock, topBlock->GetTargetEdge()); + fgRedirectTargetEdge(topBlock, InlineeCompiler->fgFirstBB); topBlock->SetNext(InlineeCompiler->fgFirstBB); - topBlock->SetTargetEdge(newEdge); topBlock->SetFlags(BBF_NONE_QUIRK); InlineeCompiler->fgLastBB->SetNext(bottomBlock); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index dc492aeaf5924..d62e7b37214a7 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -834,9 +834,7 @@ PhaseStatus Compiler::fgPostImportationCleanup() if (entryJumpTarget != osrEntry) { - fgRemoveRefPred(fgFirstBB->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(entryJumpTarget, fgFirstBB, fgFirstBB->GetTargetEdge()); - fgFirstBB->SetTargetEdge(newEdge); + fgRedirectTargetEdge(fgFirstBB, entryJumpTarget); JITDUMP("OSR: redirecting flow from method entry " FMT_BB " to OSR entry " FMT_BB " via step blocks.\n", @@ -1570,9 +1568,7 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc case BBJ_ALWAYS: case BBJ_CALLFINALLYRET: { - fgRemoveRefPred(block->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(bDest->GetTarget(), block, block->GetTargetEdge()); - block->SetTargetEdge(newEdge); + fgRedirectTargetEdge(block, bDest->GetTarget()); break; } diff --git a/src/coreclr/jit/helperexpansion.cpp b/src/coreclr/jit/helperexpansion.cpp index fa277b07db21b..b9db4a6ddc917 100644 --- a/src/coreclr/jit/helperexpansion.cpp +++ b/src/coreclr/jit/helperexpansion.cpp @@ -372,7 +372,6 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm // Update preds in all new blocks // assert(prevBb->KindIs(BBJ_ALWAYS)); - fgRemoveRefPred(prevBb->GetTargetEdge()); { FlowEdge* const newEdge = fgAddRefPred(block, fastPathBb); @@ -389,8 +388,7 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm if (needsSizeCheck) { // sizeCheckBb is the first block after prevBb - FlowEdge* const newEdge = fgAddRefPred(sizeCheckBb, prevBb); - prevBb->SetTargetEdge(newEdge); + fgRedirectTargetEdge(prevBb, sizeCheckBb); // sizeCheckBb flows into nullcheckBb in case if the size check passes { @@ -406,8 +404,7 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm else { // nullcheckBb is the first block after prevBb - FlowEdge* const newEdge = fgAddRefPred(nullcheckBb, prevBb); - prevBb->SetTargetEdge(newEdge); + fgRedirectTargetEdge(prevBb, nullcheckBb); // No size check, nullcheckBb jumps to fast path // fallbackBb is only reachable from nullcheckBb (jump destination) @@ -742,9 +739,7 @@ bool Compiler::fgExpandThreadLocalAccessForCallNativeAOT(BasicBlock** pBlock, St // fallback will just execute first time fallbackBb->bbSetRunRarely(); - fgRemoveRefPred(prevBb->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(tlsRootNullCondBB, prevBb); - prevBb->SetTargetEdge(newEdge); + fgRedirectTargetEdge(prevBb, tlsRootNullCondBB); // All blocks are expected to be in the same EH region assert(BasicBlock::sameEHRegion(prevBb, block)); @@ -1089,12 +1084,7 @@ bool Compiler::fgExpandThreadLocalAccessForCall(BasicBlock** pBlock, Statement* // Update preds in all new blocks // assert(prevBb->KindIs(BBJ_ALWAYS)); - fgRemoveRefPred(prevBb->GetTargetEdge()); - - { - FlowEdge* const newEdge = fgAddRefPred(maxThreadStaticBlocksCondBB, prevBb); - prevBb->SetTargetEdge(newEdge); - } + fgRedirectTargetEdge(prevBb, maxThreadStaticBlocksCondBB); { FlowEdge* const trueEdge = fgAddRefPred(fallbackBb, maxThreadStaticBlocksCondBB); @@ -1462,8 +1452,10 @@ bool Compiler::fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, G // Update preds in all new blocks // - // Unlink block and prevBb - fgRemoveRefPred(prevBb->GetTargetEdge()); + // Redirect prevBb from block to isInitedBb + fgRedirectTargetEdge(prevBb, isInitedBb); + prevBb->SetFlags(BBF_NONE_QUIRK); + assert(prevBb->JumpsToNext()); { // Block has two preds now: either isInitedBb or helperCallBb @@ -1473,15 +1465,6 @@ bool Compiler::fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, G helperCallBb->SetFlags(BBF_NONE_QUIRK); } - { - // prevBb always flows into isInitedBb - assert(prevBb->KindIs(BBJ_ALWAYS)); - FlowEdge* const newEdge = fgAddRefPred(isInitedBb, prevBb); - prevBb->SetTargetEdge(newEdge); - prevBb->SetFlags(BBF_NONE_QUIRK); - assert(prevBb->JumpsToNext()); - } - { // Both fastPathBb and helperCallBb have a single common pred - isInitedBb FlowEdge* const trueEdge = fgAddRefPred(block, isInitedBb); @@ -1792,17 +1775,10 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock, // // Update preds in all new blocks // - // block is no longer a predecessor of prevBb - fgRemoveRefPred(prevBb->GetTargetEdge()); - - { - // prevBb flows into lengthCheckBb - assert(prevBb->KindIs(BBJ_ALWAYS)); - FlowEdge* const newEdge = fgAddRefPred(lengthCheckBb, prevBb); - prevBb->SetTargetEdge(newEdge); - prevBb->SetFlags(BBF_NONE_QUIRK); - assert(prevBb->JumpsToNext()); - } + // Redirect prevBb to lengthCheckBb + fgRedirectTargetEdge(prevBb, lengthCheckBb); + prevBb->SetFlags(BBF_NONE_QUIRK); + assert(prevBb->JumpsToNext()); { // lengthCheckBb has two successors: block and fastpathBb @@ -2511,12 +2487,7 @@ bool Compiler::fgLateCastExpansionForCall(BasicBlock** pBlock, Statement* stmt, } } - fgRemoveRefPred(firstBb->GetTargetEdge()); - - { - FlowEdge* const newEdge = fgAddRefPred(nullcheckBb, firstBb); - firstBb->SetTargetEdge(newEdge); - } + fgRedirectTargetEdge(firstBb, nullcheckBb); { FlowEdge* const trueEdge = fgAddRefPred(lastBb, nullcheckBb); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index e2e58117a2010..931f6b3f05c8f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4683,12 +4683,15 @@ void Compiler::impImportLeave(BasicBlock* block) assert((step == block) || !step->HasInitializedTarget()); if (step == block) { - fgRemoveRefPred(step->GetTargetEdge()); + fgRedirectTargetEdge(step, exitBlock); + } + else + { + FlowEdge* const newEdge = fgAddRefPred(exitBlock, step); + step->SetTargetEdge(newEdge); // the previous step (maybe a call to a nested finally, or a nested catch + // exit) returns to this block } - FlowEdge* const newEdge = fgAddRefPred(exitBlock, step); - step->SetTargetEdge(newEdge); // the previous step (maybe a call to a nested finally, or a nested catch - // exit) returns to this block // The new block will inherit this block's weight. exitBlock->inheritWeight(block); @@ -4729,9 +4732,8 @@ void Compiler::impImportLeave(BasicBlock* block) // the new BBJ_CALLFINALLY is in a different EH region, thus it can't just replace the BBJ_LEAVE, // which might be in the middle of the "try". In most cases, the BBJ_ALWAYS will jump to the // next block, and flow optimizations will remove it. - fgRemoveRefPred(block->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(callBlock, block); - block->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); + fgRedirectTargetEdge(block, callBlock); + block->SetKind(BBJ_ALWAYS); // The new block will inherit this block's weight. callBlock->inheritWeight(block); @@ -4796,11 +4798,14 @@ void Compiler::impImportLeave(BasicBlock* block) BasicBlock* step2 = fgNewBBinRegion(BBJ_ALWAYS, XTnum + 1, 0, step); if (step == block) { - fgRemoveRefPred(step->GetTargetEdge()); + fgRedirectTargetEdge(step, step2); + } + else + { + FlowEdge* const newEdge = fgAddRefPred(step2, step); + step->SetTargetEdge(newEdge); } - FlowEdge* const newEdge = fgAddRefPred(step2, step); - step->SetTargetEdge(newEdge); step2->inheritWeight(block); step2->CopyFlags(block, BBF_RUN_RARELY); step2->SetFlags(BBF_IMPORTED); @@ -4836,12 +4841,14 @@ void Compiler::impImportLeave(BasicBlock* block) callBlock = fgNewBBinRegion(BBJ_CALLFINALLY, callFinallyTryIndex, callFinallyHndIndex, step); if (step == block) { - fgRemoveRefPred(step->GetTargetEdge()); + fgRedirectTargetEdge(step, callBlock); + } + else + { + 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 previous call to a finally returns to this call (to the next - // finally in the chain) // The new block will inherit this block's weight. callBlock->inheritWeight(block); @@ -4940,11 +4947,13 @@ void Compiler::impImportLeave(BasicBlock* block) if (step == block) { - fgRemoveRefPred(step->GetTargetEdge()); + fgRedirectTargetEdge(step, catchStep); + } + else + { + FlowEdge* const newEdge = fgAddRefPred(catchStep, step); + step->SetTargetEdge(newEdge); } - - FlowEdge* const newEdge = fgAddRefPred(catchStep, step); - step->SetTargetEdge(newEdge); // The new block will inherit this block's weight. catchStep->inheritWeight(block); @@ -4993,12 +5002,16 @@ void Compiler::impImportLeave(BasicBlock* block) { assert((step == block) || !step->HasInitializedTarget()); + // leaveTarget is the ultimate destination of the LEAVE if (step == block) { - fgRemoveRefPred(step->GetTargetEdge()); + fgRedirectTargetEdge(step, leaveTarget); + } + else + { + FlowEdge* const newEdge = fgAddRefPred(leaveTarget, step); + step->SetTargetEdge(newEdge); } - FlowEdge* const newEdge = fgAddRefPred(leaveTarget, step); - step->SetTargetEdge(newEdge); // this is the ultimate destination of the LEAVE #ifdef DEBUG if (verbose) @@ -5089,9 +5102,8 @@ void Compiler::impResetLeaveBlock(BasicBlock* block, unsigned jmpAddr) fgInitBBLookup(); - fgRemoveRefPred(block->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(fgLookupBB(jmpAddr), block); - block->SetKindAndTargetEdge(BBJ_LEAVE, newEdge); + fgRedirectTargetEdge(block, fgLookupBB(jmpAddr)); + block->SetKind(BBJ_LEAVE); // We will leave the BBJ_ALWAYS block we introduced. When it's reimported // the BBJ_ALWAYS block will be unreachable, and will be removed after. The diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 2fb3de3998d14..9220df5e3dc85 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -1219,9 +1219,7 @@ class IndirectCallTransformer // Finally, rewire the cold block to jump to the else block, // not fall through to the check block. // - compiler->fgRemoveRefPred(coldBlock->GetTargetEdge()); - FlowEdge* const newEdge = compiler->fgAddRefPred(elseBlock, coldBlock, coldBlock->GetTargetEdge()); - coldBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); + compiler->fgRedirectTargetEdge(coldBlock, elseBlock); } // When the current candidate has sufficiently high likelihood, scan diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index abef64300a5b3..ff9b449d388fc 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -4342,9 +4342,7 @@ void Compiler::fgExtendEHRegionBefore(BasicBlock* block) } #endif // DEBUG // Change the target for bFilterLast from the old first 'block' to the new first 'bPrev' - fgRemoveRefPred(bFilterLast->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(bPrev, bFilterLast); - bFilterLast->SetTargetEdge(newEdge); + fgRedirectTargetEdge(bFilterLast, bPrev); } } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index bbe9d16aa2808..5bd6a9aaf44a0 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14613,7 +14613,6 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) // Conservatively propagate BBF_COPY_PROPAGATE flags to all blocks BasicBlockFlags propagateFlagsToAll = block->GetFlagsRaw() & BBF_COPY_PROPAGATE; BasicBlock* remainderBlock = fgSplitBlockAfterStatement(block, stmt); - fgRemoveRefPred(block->GetTargetEdge()); // We're going to put more blocks between block and remainderBlock. BasicBlock* condBlock = fgNewBBafter(BBJ_ALWAYS, block, true); BasicBlock* elseBlock = fgNewBBafter(BBJ_ALWAYS, condBlock, true); @@ -14637,10 +14636,7 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) assert(condBlock->bbWeight == remainderBlock->bbWeight); assert(block->KindIs(BBJ_ALWAYS)); - { - FlowEdge* const newEdge = fgAddRefPred(condBlock, block); - block->SetTargetEdge(newEdge); - } + fgRedirectTargetEdge(block, condBlock); { FlowEdge* const newEdge = fgAddRefPred(elseBlock, condBlock); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 3fea8881bf160..7b762ee7dcb15 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2206,10 +2206,7 @@ bool Compiler::optInvertWhileLoop(BasicBlock* block) bNewCond->SetTrueEdge(trueEdge); bNewCond->SetFalseEdge(falseEdge); - fgRemoveRefPred(block->GetTargetEdge()); - FlowEdge* const newEdge = fgAddRefPred(bNewCond, block); - - block->SetTargetEdge(newEdge); + fgRedirectTargetEdge(block, bNewCond); block->SetFlags(BBF_NONE_QUIRK); assert(block->JumpsToNext()); From 371e930d155b2e11c5b7027473faaa91e89e2ee4 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 5 Mar 2024 19:58:44 -0500 Subject: [PATCH 2/7] Refactor fgGetPredInsertPoint --- src/coreclr/jit/compiler.h | 4 +++- src/coreclr/jit/fgflow.cpp | 39 +++++++++++++++++++------------------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index ea81bb360c61e..0d30aaf74a4e0 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5902,8 +5902,10 @@ class Compiler template FlowEdge* fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowEdge* oldEdge = nullptr); - void fgUpdateEdgeTarget(FlowEdge* edge, BasicBlock* newTarget); +private: + FlowEdge** fgGetPredInsertPoint(FlowEdge* edge, BasicBlock* newTarget); +public: void fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget); void fgFindBasicBlocks(); diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 87d70b72d58ce..4377aecf2c99f 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -380,17 +380,15 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block) } } -void Compiler::fgUpdateEdgeTarget(FlowEdge* edge, BasicBlock* newTarget) +FlowEdge** Compiler::fgGetPredInsertPoint(FlowEdge* edge, BasicBlock* newTarget) { assert(edge != nullptr); assert(newTarget != nullptr); assert(fgPredsComputed); - newTarget->bbRefs++; - FlowEdge** listp = &newTarget->bbPreds; - // References are added randomly, so we have to search. + // Search pred list for insertion point // BasicBlock* const blockPred = edge->getSourceBlock(); while ((*listp != nullptr) && ((*listp)->getSourceBlock()->bbNum < blockPred->bbNum)) @@ -398,15 +396,7 @@ void Compiler::fgUpdateEdgeTarget(FlowEdge* edge, BasicBlock* newTarget) listp = (*listp)->getNextPredEdgeRef(); } - // Splice new edge into the list in the correct ordered location. - // - edge->setNextPredEdge(*listp); - edge->setDestinationBlock(newTarget); - *listp = edge; - - // Pred list should (still) be ordered. - // - assert(newTarget->checkPredListOrder()); + return listp; } void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) @@ -414,15 +404,24 @@ void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) assert(block != nullptr); assert(newTarget != nullptr); - const bool fgModifiedOld = fgModified; - FlowEdge* targetEdge = block->GetTargetEdge(); - BasicBlock* oldTarget = targetEdge->getDestinationBlock(); - + FlowEdge* edge = block->GetTargetEdge(); + assert(edge->getDupCount() == 1); + + BasicBlock* oldTarget = edge->getDestinationBlock(); fgRemoveAllRefPreds(oldTarget, block); - fgUpdateEdgeTarget(targetEdge, newTarget); - assert(targetEdge->getLikelihood() == 1.0); + + // Splice edge into new target block's pred list + // + FlowEdge** predListPtr = fgGetPredInsertPoint(edge, newTarget); + edge->setNextPredEdge(*predListPtr); + edge->setDestinationBlock(newTarget); + *predListPtr = edge; + + // Pred list of target should (stilL) be ordered + // + assert(newTarget->checkPredListOrder()); - fgModified = fgModifiedOld || (newTarget != oldTarget); + newTarget->bbRefs += edge->getDupCount(); } Compiler::SwitchUniqueSuccSet Compiler::GetDescriptorForSwitch(BasicBlock* switchBlk) From 8cd1ef2fce211ba990a3a6fc6a85ebc7a831a202 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 6 Mar 2024 10:55:56 -0500 Subject: [PATCH 3/7] Reuse fgGetPredInsertPoint --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgflow.cpp | 15 ++++++--------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 0d30aaf74a4e0..57ca8d8b9d4fc 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5903,7 +5903,7 @@ class Compiler FlowEdge* fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowEdge* oldEdge = nullptr); private: - FlowEdge** fgGetPredInsertPoint(FlowEdge* edge, BasicBlock* newTarget); + FlowEdge** fgGetPredInsertPoint(BasicBlock* blockPred, BasicBlock* newTarget); public: void fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget); diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 4377aecf2c99f..6db4b90345b27 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -118,7 +118,7 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE // count of duplication. This also necessitates walking the flow list for every edge we add. // FlowEdge* flow = nullptr; - FlowEdge** listp = &block->bbPreds; + FlowEdge** listp; if (initializingPreds) { @@ -126,6 +126,7 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE // increasing blockPred->bbNum order. The only possible // dup list entry is the last one. // + listp = &block->bbPreds; FlowEdge* flowLast = block->bbLastPred; if (flowLast != nullptr) { @@ -143,10 +144,7 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE { // References are added randomly, so we have to search. // - while ((*listp != nullptr) && ((*listp)->getSourceBlock()->bbNum < blockPred->bbNum)) - { - listp = (*listp)->getNextPredEdgeRef(); - } + listp = fgGetPredInsertPoint(blockPred, block); if ((*listp != nullptr) && ((*listp)->getSourceBlock() == blockPred)) { @@ -380,9 +378,9 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block) } } -FlowEdge** Compiler::fgGetPredInsertPoint(FlowEdge* edge, BasicBlock* newTarget) +FlowEdge** Compiler::fgGetPredInsertPoint(BasicBlock* blockPred, BasicBlock* newTarget) { - assert(edge != nullptr); + assert(blockPred != nullptr); assert(newTarget != nullptr); assert(fgPredsComputed); @@ -390,7 +388,6 @@ FlowEdge** Compiler::fgGetPredInsertPoint(FlowEdge* edge, BasicBlock* newTarget) // Search pred list for insertion point // - BasicBlock* const blockPred = edge->getSourceBlock(); while ((*listp != nullptr) && ((*listp)->getSourceBlock()->bbNum < blockPred->bbNum)) { listp = (*listp)->getNextPredEdgeRef(); @@ -412,7 +409,7 @@ void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) // Splice edge into new target block's pred list // - FlowEdge** predListPtr = fgGetPredInsertPoint(edge, newTarget); + FlowEdge** predListPtr = fgGetPredInsertPoint(block, newTarget); edge->setNextPredEdge(*predListPtr); edge->setDestinationBlock(newTarget); *predListPtr = edge; From 44c1042a39a54af0dfd4addeda0758907c053bb5 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 6 Mar 2024 11:15:38 -0500 Subject: [PATCH 4/7] Add comments --- src/coreclr/jit/fgbasic.cpp | 1 + src/coreclr/jit/fgflow.cpp | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 9a86ac91875c7..0eb618f177722 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -646,6 +646,7 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas case BBJ_EHFILTERRET: case BBJ_LEAVE: // This function can be called before import, so we still have BBJ_LEAVE { + // TODO: Use fgRedirectTargetEdge once pred edge iterators are resilient to bbPreds being modified. assert(block->TargetIs(oldTarget)); fgRemoveRefPred(block->GetTargetEdge()); FlowEdge* const newEdge = fgAddRefPred(newTarget, block, block->GetTargetEdge()); diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 6db4b90345b27..0137edccd47e8 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -378,6 +378,18 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block) } } +//------------------------------------------------------------------------ +// fgGetPredInsertPoint: Searches newTarget->bbPreds for where to insert an edge from blockPred. +// +// Arguments: +// blockPred -- The block we want to make a predecessor of newTarget (it could already be one). +// newTarget -- The block whose pred list we will search. +// +// Return Value: +// Returns a pointer to the next pointer of an edge in newTarget's pred list. +// A new edge from blockPred to newTarget can be inserted here +// without disrupting bbPreds' sorting invariant. +// FlowEdge** Compiler::fgGetPredInsertPoint(BasicBlock* blockPred, BasicBlock* newTarget) { assert(blockPred != nullptr); @@ -396,6 +408,15 @@ FlowEdge** Compiler::fgGetPredInsertPoint(BasicBlock* blockPred, BasicBlock* new return listp; } +//------------------------------------------------------------------------ +// fgRedirectTargetEdge: Sets block->bbTargetEdge'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. +// void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) { assert(block != nullptr); @@ -404,6 +425,10 @@ void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) FlowEdge* edge = block->GetTargetEdge(); assert(edge->getDupCount() == 1); + // 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). + // BasicBlock* oldTarget = edge->getDestinationBlock(); fgRemoveAllRefPreds(oldTarget, block); From c2d55729c891aca6c9ededdad3b76d332a3233cb Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 6 Mar 2024 11:16:26 -0500 Subject: [PATCH 5/7] Style --- src/coreclr/jit/fgehopt.cpp | 1 - src/coreclr/jit/fgflow.cpp | 10 +++++----- src/coreclr/jit/importer.cpp | 4 ++-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 6b5d286841255..743841fec2429 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -2127,7 +2127,6 @@ void Compiler::fgTailMergeThrowsJumpToHelper(BasicBlock* predBlock, { JITDUMP("*** " FMT_BB " now branching to " FMT_BB "\n", predBlock->bbNum, canonicalBlock->bbNum); - if (predBlock->KindIs(BBJ_ALWAYS)) { // Update flow to new target diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 0137edccd47e8..1400941472362 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -117,7 +117,7 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE // dependency on this order. Note also that we don't allow duplicates in the list; we maintain a DupCount // count of duplication. This also necessitates walking the flow list for every edge we add. // - FlowEdge* flow = nullptr; + FlowEdge* flow = nullptr; FlowEdge** listp; if (initializingPreds) @@ -126,7 +126,7 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE // increasing blockPred->bbNum order. The only possible // dup list entry is the last one. // - listp = &block->bbPreds; + listp = &block->bbPreds; FlowEdge* flowLast = block->bbLastPred; if (flowLast != nullptr) { @@ -395,7 +395,7 @@ FlowEdge** Compiler::fgGetPredInsertPoint(BasicBlock* blockPred, BasicBlock* new assert(blockPred != nullptr); assert(newTarget != nullptr); assert(fgPredsComputed); - + FlowEdge** listp = &newTarget->bbPreds; // Search pred list for insertion point @@ -424,14 +424,14 @@ void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) FlowEdge* edge = block->GetTargetEdge(); assert(edge->getDupCount() == 1); - + // 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). // BasicBlock* oldTarget = edge->getDestinationBlock(); fgRemoveAllRefPreds(oldTarget, block); - + // Splice edge into new target block's pred list // FlowEdge** predListPtr = fgGetPredInsertPoint(block, newTarget); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 931f6b3f05c8f..e50773eefd4a5 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4688,11 +4688,11 @@ void Compiler::impImportLeave(BasicBlock* block) else { FlowEdge* const newEdge = fgAddRefPred(exitBlock, step); - step->SetTargetEdge(newEdge); // the previous step (maybe a call to a nested finally, or a nested catch + step->SetTargetEdge(newEdge); // the previous step (maybe a call to a nested finally, or a nested + // catch // exit) returns to this block } - // The new block will inherit this block's weight. exitBlock->inheritWeight(block); exitBlock->SetFlags(BBF_IMPORTED); From 6e3edcec01baf190fc0fb3852e19b4e202a101bf Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 6 Mar 2024 11:37:01 -0500 Subject: [PATCH 6/7] More cleanup --- src/coreclr/jit/fgflow.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 1400941472362..5ba48f87ce278 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -443,7 +443,9 @@ void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) // assert(newTarget->checkPredListOrder()); - newTarget->bbRefs += edge->getDupCount(); + // Edge should still have only one ref + assert(edge->getDupCount() == 1); + newTarget->bbRefs++; } Compiler::SwitchUniqueSuccSet Compiler::GetDescriptorForSwitch(BasicBlock* switchBlk) From 9fd3f43f40218c65a48819dfc817eb807c4e3317 Mon Sep 17 00:00:00 2001 From: Aman Khalid Date: Thu, 7 Mar 2024 16:13:32 -0500 Subject: [PATCH 7/7] Fix typo Co-authored-by: Andy Ayers --- src/coreclr/jit/fgflow.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 5ba48f87ce278..6e39f2fb24dfe 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -439,7 +439,7 @@ void Compiler::fgRedirectTargetEdge(BasicBlock* block, BasicBlock* newTarget) edge->setDestinationBlock(newTarget); *predListPtr = edge; - // Pred list of target should (stilL) be ordered + // Pred list of target should (still) be ordered // assert(newTarget->checkPredListOrder());