From 540ebac2c7fb019caaa126f55a5b950a32a4f1d1 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 21 Dec 2023 15:24:14 -0500 Subject: [PATCH 1/7] Move bbFalseTarget update from SetNext to fgAddRefPred --- src/coreclr/jit/block.cpp | 2 +- src/coreclr/jit/block.h | 11 +++-------- src/coreclr/jit/fgbasic.cpp | 7 +++++++ src/coreclr/jit/fgflow.cpp | 5 +++++ src/coreclr/jit/fgopt.cpp | 1 + src/coreclr/jit/flowgraph.cpp | 22 ++++++++++------------ src/coreclr/jit/helperexpansion.cpp | 19 +++++++++++++------ src/coreclr/jit/optimizebools.cpp | 6 ++++-- src/coreclr/jit/optimizer.cpp | 4 +++- 9 files changed, 47 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index c95a8a72b2528f..493a2bbb827429 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -1070,7 +1070,7 @@ bool BasicBlock::bbFallsThrough() const return false; case BBJ_COND: - return NextIs(GetFalseTarget()); + return true; case BBJ_CALLFINALLY: return !HasFlag(BBF_RETLESS_CALL); diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 4d99b15b0d1835..efbace76ddfbbd 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -584,12 +584,6 @@ struct BasicBlock : private LIR::Range { next->bbPrev = this; } - - // BBJ_COND convenience: This ensures bbFalseTarget is always consistent with bbNext. - // For now, if a BBJ_COND's bbTrueTarget is not taken, we expect to fall through, - // so bbFalseTarget must be the next block. - // TODO-NoFallThrough: Remove this once we allow bbFalseTarget to diverge from bbNext - bbFalseTarget = next; } bool IsFirst() const @@ -710,8 +704,9 @@ struct BasicBlock : private LIR::Range void SetCond(BasicBlock* target) { assert(target != nullptr); - bbKind = BBJ_COND; - bbTrueTarget = target; + bbKind = BBJ_COND; + bbTrueTarget = target; + bbFalseTarget = bbNext; } void SetKindAndTarget(BBKinds kind, BasicBlock* target = nullptr) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 675b7dc9d71ec1..b5fa7da7868b5d 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -4770,6 +4770,7 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr) { case BBJ_COND: newBlock = BasicBlock::New(this, BBJ_COND, curr->GetTrueTarget()); + newBlock->SetFalseTarget(curr->GetFalseTarget()); break; case BBJ_EHFINALLYRET: @@ -5467,6 +5468,8 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable) break; case BBJ_COND: + bPrev->SetFalseTarget(block->Next()); + /* Check if both sides of the BBJ_COND now jump to the same block */ if (bPrev->TrueTargetIs(bPrev->GetFalseTarget())) { @@ -5616,6 +5619,10 @@ BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst) { bSrc->SetFlags(BBF_NONE_QUIRK); } + else if (bSrc->KindIs(BBJ_COND) && bSrc->NextIs(bDst)) + { + bSrc->SetFalseTarget(bDst); + } } return jmpBlk; diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 5f6a754a99416c..b7242e71a448f6 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -104,6 +104,11 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE block->bbRefs++; + if (blockPred->KindIs(BBJ_COND) && blockPred->NextIs(block)) + { + blockPred->SetFalseTarget(block); + } + // Keep the predecessor list in lowest to highest bbNum order. This allows us to discover the loops in // optFindNaturalLoops from innermost to outermost. // diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 958d4c2587b919..c86000a174aa4b 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6234,6 +6234,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) // Optimize the Conditional JUMP to go to the new target block->SetTrueTarget(bNext->GetTarget()); + block->SetFalseTarget(bNext->Next()); fgAddRefPred(bNext->GetTarget(), block, fgRemoveRefPred(bNext->GetTarget(), bNext)); diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 86c9c6069b6d1e..10a1d77dd407ce 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -267,26 +267,24 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block) // I want to create: // top -> poll -> bottom (lexically) // so that we jump over poll to get to bottom. - BasicBlock* top = block; - BasicBlock* topFallThrough = nullptr; - BasicBlock* topJumpTarget; + BasicBlock* top = block; unsigned char lpIndexFallThrough = BasicBlock::NOT_IN_LOOP; - if (top->KindIs(BBJ_COND)) + BasicBlock* poll = fgNewBBafter(BBJ_ALWAYS, top, true); + BBKinds oldJumpKind = top->GetKind(); + unsigned char lpIndex = top->bbNatLoopNum; + + if (oldJumpKind == BBJ_COND) { - topFallThrough = top->GetFalseTarget(); - topJumpTarget = top->GetTrueTarget(); - lpIndexFallThrough = topFallThrough->bbNatLoopNum; + lpIndexFallThrough = top->GetFalseTarget()->bbNatLoopNum; + bottom = fgNewBBafter(BBJ_COND, poll, true, top->GetTrueTarget()); + bottom->SetFalseTarget(bottom->Next()); } else { - topJumpTarget = top->GetTarget(); + bottom = fgNewBBafter(oldJumpKind, poll, true, top->GetTarget()); } - BasicBlock* poll = fgNewBBafter(BBJ_ALWAYS, top, true); - bottom = fgNewBBafter(top->GetKind(), poll, true, topJumpTarget); - BBKinds oldJumpKind = top->GetKind(); - unsigned char lpIndex = top->bbNatLoopNum; poll->SetTarget(bottom); assert(poll->JumpsToNext()); diff --git a/src/coreclr/jit/helperexpansion.cpp b/src/coreclr/jit/helperexpansion.cpp index 7b4c4fe4610d96..bcc6982d8e1b87 100644 --- a/src/coreclr/jit/helperexpansion.cpp +++ b/src/coreclr/jit/helperexpansion.cpp @@ -286,7 +286,7 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm // Fallback basic block GenTree* fallbackValueDef = gtNewStoreLclVarNode(rtLookupLcl->GetLclNum(), call); BasicBlock* fallbackBb = - fgNewBBFromTreeAfter(BBJ_ALWAYS, nullcheckBb, fallbackValueDef, debugInfo, nullcheckBb->GetFalseTarget(), true); + fgNewBBFromTreeAfter(BBJ_ALWAYS, nullcheckBb, fallbackValueDef, debugInfo, nullcheckBb->Next(), true); assert(fallbackBb->JumpsToNext()); fallbackBb->SetFlags(BBF_NONE_QUIRK); @@ -298,6 +298,9 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm GenTree* fastpathValueDef = gtNewStoreLclVarNode(rtLookupLcl->GetLclNum(), fastPathValueClone); BasicBlock* fastPathBb = fgNewBBFromTreeAfter(BBJ_ALWAYS, nullcheckBb, fastpathValueDef, debugInfo, block); + // Set nullcheckBb's false jump target + nullcheckBb->SetFalseTarget(fastPathBb); + BasicBlock* sizeCheckBb = nullptr; if (needsSizeCheck) { @@ -339,6 +342,7 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm GenTree* jtrue = gtNewOperNode(GT_JTRUE, TYP_VOID, sizeCheck); // sizeCheckBb fails - jump to fallbackBb sizeCheckBb = fgNewBBFromTreeAfter(BBJ_COND, prevBb, jtrue, debugInfo, fallbackBb); + sizeCheckBb->SetFalseTarget(nullcheckBb); } // @@ -780,11 +784,13 @@ bool Compiler::fgExpandThreadLocalAccessForCall(BasicBlock** pBlock, Statement* gtNewStoreLclVarNode(threadStaticBlockLclNum, gtCloneExpr(threadStaticBlockBaseLclValueUse)); BasicBlock* fastPathBb = fgNewBBFromTreeAfter(BBJ_ALWAYS, fallbackBb, fastPathValueDef, debugInfo, block, true); - // Set maxThreadStaticBlocksCondBB's true jump target + // Set maxThreadStaticBlocksCondBB's jump targets maxThreadStaticBlocksCondBB->SetTrueTarget(fallbackBb); + maxThreadStaticBlocksCondBB->SetFalseTarget(threadStaticBlockNullCondBB); - // Set threadStaticBlockNullCondBB's true jump target + // Set threadStaticBlockNullCondBB's jump targets threadStaticBlockNullCondBB->SetTrueTarget(fastPathBb); + threadStaticBlockNullCondBB->SetFalseTarget(fallbackBb); // // Update preds in all new blocks @@ -1095,8 +1101,7 @@ bool Compiler::fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, G // Fallback basic block // TODO-CQ: for JIT we can replace the original call with CORINFO_HELP_INITCLASS // that only accepts a single argument - BasicBlock* helperCallBb = - fgNewBBFromTreeAfter(BBJ_ALWAYS, isInitedBb, call, debugInfo, isInitedBb->GetFalseTarget(), true); + BasicBlock* helperCallBb = fgNewBBFromTreeAfter(BBJ_ALWAYS, isInitedBb, call, debugInfo, isInitedBb->Next(), true); assert(helperCallBb->JumpsToNext()); helperCallBb->SetFlags(BBF_NONE_QUIRK); @@ -1172,6 +1177,7 @@ bool Compiler::fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, G fgAddRefPred(isInitedBb, prevBb); // Both fastPathBb and helperCallBb have a single common pred - isInitedBb + isInitedBb->SetFalseTarget(helperCallBb); fgAddRefPred(helperCallBb, isInitedBb); // @@ -1451,7 +1457,7 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock, // In theory, we could just emit the const U8 data to the data section and use GT_BLK here // but that would be a bit less efficient since we would have to load the data from memory. // - BasicBlock* fastpathBb = fgNewBBafter(BBJ_ALWAYS, lengthCheckBb, true, lengthCheckBb->GetFalseTarget()); + BasicBlock* fastpathBb = fgNewBBafter(BBJ_ALWAYS, lengthCheckBb, true, lengthCheckBb->Next()); assert(fastpathBb->JumpsToNext()); fastpathBb->SetFlags(BBF_INTERNAL | BBF_NONE_QUIRK); @@ -1514,6 +1520,7 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock, assert(prevBb->JumpsToNext()); fgAddRefPred(lengthCheckBb, prevBb); // lengthCheckBb has two successors: block and fastpathBb + lengthCheckBb->SetFalseTarget(fastpathBb); fgAddRefPred(fastpathBb, lengthCheckBb); fgAddRefPred(block, lengthCheckBb); // fastpathBb flows into block diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index e5d366548e53aa..b96c4ae024e404 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1298,6 +1298,7 @@ void OptBoolsDsc::optOptimizeBoolsUpdateTrees() if (optReturnBlock) { + assert(m_b1->KindIs(BBJ_COND)); assert(m_b2->KindIs(BBJ_RETURN)); assert(m_b1->FalseTargetIs(m_b2)); assert(m_b3 != nullptr); @@ -1316,10 +1317,11 @@ void OptBoolsDsc::optOptimizeBoolsUpdateTrees() { // Update bbRefs and bbPreds // - // Replace pred 'm_b2' for 'm_b2->bbNext' with 'm_b1' - // Remove pred 'm_b2' for 'm_b2->bbTarget' + // Replace pred 'm_b2' for 'm_b2->bbFalseTarget' with 'm_b1' + // Remove pred 'm_b2' for 'm_b2->bbTrueTarget' m_comp->fgReplacePred(m_b2->GetFalseTarget(), m_b2, m_b1); m_comp->fgRemoveRefPred(m_b2->GetTrueTarget(), m_b2); + m_b1->SetFalseTarget(m_b2->GetFalseTarget()); } // Get rid of the second block diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index aabf07aea4868a..db957b2c9264d4 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2270,6 +2270,7 @@ class LoopSearch // Redirect the Conditional JUMP to go to `oldNext` block->SetTrueTarget(oldNext); + block->SetFalseTarget(newNext); } else { @@ -8227,7 +8228,8 @@ bool Compiler::fgCreateLoopPreHeader(unsigned lnum) } else { - noway_assert((entry == top) && (predBlock == head) && predBlock->FalseTargetIs(preHead)); + noway_assert((entry == top) && (predBlock == head) && predBlock->NextIs(preHead)); + predBlock->SetFalseTarget(preHead); } fgRemoveRefPred(entry, predBlock); fgAddRefPred(preHead, predBlock); From bc3f3dbcd818fb2e9f64eb84c8076f0d9195f083 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 21 Dec 2023 18:28:01 -0500 Subject: [PATCH 2/7] Remove SetFalseTarget logic in fgAddRefPred --- src/coreclr/jit/fgbasic.cpp | 2 ++ src/coreclr/jit/fgehopt.cpp | 1 + src/coreclr/jit/fgflow.cpp | 5 ----- src/coreclr/jit/fgopt.cpp | 2 ++ src/coreclr/jit/jiteh.cpp | 3 ++- src/coreclr/jit/loopcloning.cpp | 12 +++++++----- src/coreclr/jit/morph.cpp | 6 +++++- src/coreclr/jit/optimizer.cpp | 11 +++++++++-- 8 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index b5fa7da7868b5d..62d41b24c5f033 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -5093,6 +5093,7 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ) if (curr->KindIs(BBJ_COND)) { + curr->SetFalseTarget(curr->Next()); fgReplacePred(succ, curr, newBlock); if (curr->TrueTargetIs(succ)) { @@ -5556,6 +5557,7 @@ BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst) // Add a new block after bSrc which jumps to 'bDst' jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst); + bSrc->SetFalseTarget(jmpBlk); fgAddRefPred(jmpBlk, bSrc, fgGetPredForBlock(bDst, bSrc)); // Record the loop number in the new block diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index c6b6fa56884418..a4c38e122e2b77 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -2118,6 +2118,7 @@ void Compiler::fgTailMergeThrowsFallThroughHelper(BasicBlock* predBlock, assert(predBlock->FalseTargetIs(nonCanonicalBlock)); BasicBlock* const newBlock = fgNewBBafter(BBJ_ALWAYS, predBlock, true, canonicalBlock); + predBlock->SetFalseTarget(newBlock); JITDUMP("*** " FMT_BB " now falling through to empty " FMT_BB " and then to " FMT_BB "\n", predBlock->bbNum, newBlock->bbNum, canonicalBlock->bbNum); diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index b7242e71a448f6..5f6a754a99416c 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -104,11 +104,6 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE block->bbRefs++; - if (blockPred->KindIs(BBJ_COND) && blockPred->NextIs(block)) - { - blockPred->SetFalseTarget(block); - } - // Keep the predecessor list in lowest to highest bbNum order. This allows us to discover the loops in // optFindNaturalLoops from innermost to outermost. // diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c86000a174aa4b..9e7ebb5f7275ff 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3713,6 +3713,7 @@ bool Compiler::fgOptimizeUncondBranchToSimpleCond(BasicBlock* block, BasicBlock* // assert(!target->IsLast()); BasicBlock* next = fgNewBBafter(BBJ_ALWAYS, block, true, target->GetFalseTarget()); + block->SetFalseTarget(next); // The new block 'next' will inherit its weight from 'block' // @@ -6199,6 +6200,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) if (bDest->KindIs(BBJ_COND)) { BasicBlock* const bFixup = fgNewBBafter(BBJ_ALWAYS, bDest, true, bDestNext); + bDest->SetFalseTarget(bFixup); bFixup->inheritWeight(bDestNext); fgRemoveRefPred(bDestNext, bDest); diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 846bf7032e2020..f1bdeeb22265fd 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -2256,8 +2256,9 @@ bool Compiler::fgNormalizeEHCase2() // fgReplaceJumpTarget(predBlock, newTryStart, insertBeforeBlk); - if (predBlock->NextIs(newTryStart) && predBlock->bbFallsThrough()) + if (predBlock->NextIs(newTryStart) && predBlock->KindIs(BBJ_COND)) { + predBlock->SetFalseTarget(newTryStart); fgRemoveRefPred(insertBeforeBlk, predBlock); fgAddRefPred(newTryStart, predBlock); } diff --git a/src/coreclr/jit/loopcloning.cpp b/src/coreclr/jit/loopcloning.cpp index 7b85126a20422b..5e6c1b89ace0e3 100644 --- a/src/coreclr/jit/loopcloning.cpp +++ b/src/coreclr/jit/loopcloning.cpp @@ -867,9 +867,10 @@ BasicBlock* LoopCloneContext::CondToStmtInBlock(Compiler* JITDUMP("Adding " FMT_BB " -> " FMT_BB "\n", newBlk->bbNum, newBlk->GetTrueTarget()->bbNum); comp->fgAddRefPred(newBlk->GetTrueTarget(), newBlk); - if (insertAfter->bbFallsThrough()) + if (insertAfter->KindIs(BBJ_COND)) { JITDUMP("Adding " FMT_BB " -> " FMT_BB "\n", insertAfter->bbNum, newBlk->bbNum); + insertAfter->SetFalseTarget(newBlk); comp->fgAddRefPred(newBlk, insertAfter); } @@ -2081,12 +2082,11 @@ void Compiler::optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* contex // bottomNext BasicBlock* bottom = loop->GetLexicallyBottomMostBlock(); BasicBlock* newPred = bottom; - if (bottom->bbFallsThrough()) + if (bottom->KindIs(BBJ_COND)) { - // Once bbFalseTarget can diverge from bbNext, BBJ_COND blocks will "fall through" - // only if bbFalseTarget is still the next block - assert(!bottom->KindIs(BBJ_COND) || bottom->NextIs(bottom->GetFalseTarget())); + // TODO-NoFallThrough: Shouldn't need new BBJ_ALWAYS block once bbFalseTarget can diverge from bbNext BasicBlock* bottomNext = bottom->Next(); + assert(bottom->FalseTargetIs(bottomNext)); JITDUMP("Create branch around cloned loop\n"); BasicBlock* bottomRedirBlk = fgNewBBafter(BBJ_ALWAYS, bottom, /*extendRegion*/ true, bottomNext); JITDUMP("Adding " FMT_BB " after " FMT_BB "\n", bottomRedirBlk->bbNum, bottom->bbNum); @@ -2095,6 +2095,7 @@ void Compiler::optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* contex // This is in the scope of a surrounding loop, if one exists -- the parent of the loop we're cloning. bottomRedirBlk->bbNatLoopNum = ambientLoop; + bottom->SetFalseTarget(bottomRedirBlk); fgAddRefPred(bottomRedirBlk, bottom); JITDUMP("Adding " FMT_BB " -> " FMT_BB "\n", bottom->bbNum, bottomRedirBlk->bbNum); fgReplacePred(bottomNext, bottom, bottomRedirBlk); @@ -2306,6 +2307,7 @@ void Compiler::optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* contex // And make sure we insert a pred link for the final fallthrough into the fast preheader. assert(condLast->NextIs(fastPreheader)); + condLast->SetFalseTarget(fastPreheader); fgAddRefPred(fastPreheader, condLast); // Don't unroll loops that we've cloned -- the unroller expects any loop it should unroll to diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 75f885ac8d9909..c591ba0c129976 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14677,9 +14677,12 @@ bool Compiler::fgExpandQmarkForCastInstOf(BasicBlock* block, Statement* stmt) block->SetTarget(asgBlock); fgAddRefPred(asgBlock, block); fgAddRefPred(cond1Block, asgBlock); + fgAddRefPred(remainderBlock, helperBlock); + + cond1Block->SetFalseTarget(cond2Block); + cond2Block->SetFalseTarget(helperBlock); fgAddRefPred(cond2Block, cond1Block); fgAddRefPred(helperBlock, cond2Block); - fgAddRefPred(remainderBlock, helperBlock); fgAddRefPred(remainderBlock, cond1Block); fgAddRefPred(remainderBlock, cond2Block); @@ -14902,6 +14905,7 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) thenBlock = fgNewBBafter(BBJ_ALWAYS, condBlock, true, remainderBlock); thenBlock->SetFlags(propagateFlagsToAll); + condBlock->SetFalseTarget(thenBlock); if (!block->HasFlag(BBF_INTERNAL)) { thenBlock->RemoveFlags(BBF_INTERNAL); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index db957b2c9264d4..ddee96af735b45 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2982,6 +2982,7 @@ bool Compiler::optCanonicalizeLoop(unsigned char loopInd) fgSetEHRegionForNewLoopHead(newH, t); + h->SetFalseTarget(newH); fgRemoveRefPred(t, h); fgAddRefPred(t, newH); fgAddRefPred(newH, h); @@ -3163,6 +3164,7 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati { BasicBlock* const hj = h->GetTrueTarget(); assert((hj->bbNum < t->bbNum) || (hj->bbNum > b->bbNum)); + h->SetFalseTarget(newT); } else { @@ -4374,15 +4376,19 @@ PhaseStatus Compiler::optUnrollLoops() // BasicBlock* const clonedTop = blockMap[loop.lpTop]; BasicBlock* clonedTopPrev = clonedTop->Prev(); - assert(clonedTopPrev->KindIs(BBJ_ALWAYS, BBJ_COND)); if (clonedTopPrev->KindIs(BBJ_ALWAYS)) { assert(!clonedTopPrev->HasInitializedTarget()); clonedTopPrev->SetTarget(clonedTop); } + else + { + assert(clonedTopPrev->KindIs(BBJ_COND)); + clonedTopPrev->SetFalseTarget(clonedTop); + } - fgAddRefPred(clonedTop, clonedTop->Prev()); + fgAddRefPred(clonedTop, clonedTopPrev); /* update the new value for the unrolled iterator */ @@ -5024,6 +5030,7 @@ bool Compiler::optInvertWhileLoop(BasicBlock* block) // Update pred info // + bNewCond->SetFalseTarget(bTop); fgAddRefPred(bJoin, bNewCond); fgAddRefPred(bTop, bNewCond); From 98f2cb730eb0275db4fd6b8320f1a239d67fcc6a Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 22 Dec 2023 12:56:15 -0500 Subject: [PATCH 3/7] Fix assert on arm --- src/coreclr/jit/lower.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index adee5fd7456255..db6cd3e7b3985f 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1074,6 +1074,7 @@ GenTree* Lowering::LowerSwitch(GenTree* node) { BasicBlock* newBlock = comp->fgNewBBafter(BBJ_ALWAYS, currentBlock, true, currentBlock->Next()); newBlock->SetFlags(BBF_NONE_QUIRK); + currentBlock->SetFalseTarget(newBlock); comp->fgAddRefPred(newBlock, currentBlock); // The fall-through predecessor. currentBlock = newBlock; currentBBRange = &LIR::AsRange(currentBlock); From 77c92359124ba84cb2e330020d129363d489f370 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 22 Dec 2023 14:26:11 -0500 Subject: [PATCH 4/7] Small cleanup --- src/coreclr/jit/fgopt.cpp | 12 +++++------- src/coreclr/jit/lower.cpp | 9 +++------ src/coreclr/jit/morph.cpp | 3 +-- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 9e7ebb5f7275ff..5a5fe92155c4e2 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3703,18 +3703,17 @@ bool Compiler::fgOptimizeUncondBranchToSimpleCond(BasicBlock* block, BasicBlock* fgInsertStmtAtEnd(block, cloneStmt); } + // add an unconditional block after this block to jump to the target block's fallthrough block + // + assert(!target->IsLast()); + BasicBlock* next = fgNewBBafter(BBJ_ALWAYS, block, true, target->GetFalseTarget()); + // Fix up block's flow // block->SetCond(target->GetTrueTarget()); fgAddRefPred(block->GetTrueTarget(), block); fgRemoveRefPred(target, block); - // add an unconditional block after this block to jump to the target block's fallthrough block - // - assert(!target->IsLast()); - BasicBlock* next = fgNewBBafter(BBJ_ALWAYS, block, true, target->GetFalseTarget()); - block->SetFalseTarget(next); - // The new block 'next' will inherit its weight from 'block' // next->inheritWeight(block); @@ -4119,7 +4118,6 @@ bool Compiler::fgOptimizeBranch(BasicBlock* bJump) bJump->CopyFlags(bDest, BBF_COPY_PROPAGATE); bJump->SetCond(bDestNormalTarget); - bJump->SetFalseTarget(bJump->Next()); /* Update bbRefs and bbPreds */ diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index db6cd3e7b3985f..26addf635a8ec9 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1314,9 +1314,6 @@ bool Lowering::TryLowerSwitchToBitTest( // GenCondition::C generates JC so we jump to bbCase1 when the bit is set bbSwitchCondition = GenCondition::C; bbSwitch->SetCond(bbCase1); - - comp->fgAddRefPred(bbCase0, bbSwitch); - comp->fgAddRefPred(bbCase1, bbSwitch); } else { @@ -1325,11 +1322,11 @@ bool Lowering::TryLowerSwitchToBitTest( // GenCondition::NC generates JNC so we jump to bbCase0 when the bit is not set bbSwitchCondition = GenCondition::NC; bbSwitch->SetCond(bbCase0); - - comp->fgAddRefPred(bbCase0, bbSwitch); - comp->fgAddRefPred(bbCase1, bbSwitch); } + comp->fgAddRefPred(bbCase0, bbSwitch); + comp->fgAddRefPred(bbCase1, bbSwitch); + var_types bitTableType = (bitCount <= (genTypeSize(TYP_INT) * 8)) ? TYP_INT : TYP_LONG; GenTree* bitTableIcon = comp->gtNewIconNode(bitTable, bitTableType); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index c591ba0c129976..2d323638ab028b 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14901,11 +14901,10 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) // bbj_cond(true) // gtReverseCond(condExpr); - condBlock->SetCond(elseBlock); thenBlock = fgNewBBafter(BBJ_ALWAYS, condBlock, true, remainderBlock); thenBlock->SetFlags(propagateFlagsToAll); - condBlock->SetFalseTarget(thenBlock); + condBlock->SetCond(elseBlock); if (!block->HasFlag(BBF_INTERNAL)) { thenBlock->RemoveFlags(BBF_INTERNAL); From 6a755ff16a9ff45f8f10dd18cdd2a80d8f2dc53a Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 22 Dec 2023 17:51:08 -0500 Subject: [PATCH 5/7] Remove assert from fgAddRefPred --- src/coreclr/jit/fgflow.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 37e9e8a1b51046..5f6a754a99416c 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -104,11 +104,6 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE block->bbRefs++; - if (blockPred->KindIs(BBJ_COND) && !blockPred->TrueTargetIs(block) && blockPred->NextIs(block)) - { - assert(blockPred->FalseTargetIs(block)); - } - // Keep the predecessor list in lowest to highest bbNum order. This allows us to discover the loops in // optFindNaturalLoops from innermost to outermost. // From 46894c95f3692f00fa160193b08df53b9a3f0abc Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 22 Dec 2023 17:59:26 -0500 Subject: [PATCH 6/7] Clean up fgConnectFallThrough --- src/coreclr/jit/fgbasic.cpp | 109 ++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 60 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 42e027e4421ce5..24f1a282c6f603 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -5534,74 +5534,63 @@ BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst) { /* If bSrc falls through to a block that is not bDst, we will insert a jump to bDst */ - if (bSrc->bbFallsThrough() && !bSrc->NextIs(bDst)) + if (bSrc->KindIs(BBJ_COND) && !bSrc->NextIs(bDst)) { - switch (bSrc->GetKind()) - { - case BBJ_CALLFINALLY: - case BBJ_COND: + // Add a new block after bSrc which jumps to 'bDst' + jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst); + bSrc->SetFalseTarget(jmpBlk); + fgAddRefPred(jmpBlk, bSrc, fgGetPredForBlock(bDst, bSrc)); - // Add a new block after bSrc which jumps to 'bDst' - jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst); - bSrc->SetFalseTarget(jmpBlk); - fgAddRefPred(jmpBlk, bSrc, fgGetPredForBlock(bDst, bSrc)); - - // Record the loop number in the new block - jmpBlk->bbNatLoopNum = bSrc->bbNatLoopNum; - - // When adding a new jmpBlk we will set the bbWeight and bbFlags - // - if (fgHaveValidEdgeWeights && fgHaveProfileWeights()) - { - FlowEdge* const newEdge = fgGetPredForBlock(jmpBlk, bSrc); + // Record the loop number in the new block + jmpBlk->bbNatLoopNum = bSrc->bbNatLoopNum; - jmpBlk->bbWeight = (newEdge->edgeWeightMin() + newEdge->edgeWeightMax()) / 2; - if (bSrc->bbWeight == BB_ZERO_WEIGHT) - { - jmpBlk->bbWeight = BB_ZERO_WEIGHT; - } + // When adding a new jmpBlk we will set the bbWeight and bbFlags + // + if (fgHaveValidEdgeWeights && fgHaveProfileWeights()) + { + FlowEdge* const newEdge = fgGetPredForBlock(jmpBlk, bSrc); - if (jmpBlk->bbWeight == BB_ZERO_WEIGHT) - { - jmpBlk->SetFlags(BBF_RUN_RARELY); - } + jmpBlk->bbWeight = (newEdge->edgeWeightMin() + newEdge->edgeWeightMax()) / 2; + if (bSrc->bbWeight == BB_ZERO_WEIGHT) + { + jmpBlk->bbWeight = BB_ZERO_WEIGHT; + } - weight_t weightDiff = (newEdge->edgeWeightMax() - newEdge->edgeWeightMin()); - weight_t slop = BasicBlock::GetSlopFraction(bSrc, bDst); - // - // If the [min/max] values for our edge weight is within the slop factor - // then we will set the BBF_PROF_WEIGHT flag for the block - // - if (weightDiff <= slop) - { - jmpBlk->SetFlags(BBF_PROF_WEIGHT); - } - } - else - { - // We set the bbWeight to the smaller of bSrc->bbWeight or bDst->bbWeight - if (bSrc->bbWeight < bDst->bbWeight) - { - jmpBlk->bbWeight = bSrc->bbWeight; - jmpBlk->CopyFlags(bSrc, BBF_RUN_RARELY); - } - else - { - jmpBlk->bbWeight = bDst->bbWeight; - jmpBlk->CopyFlags(bDst, BBF_RUN_RARELY); - } - } + if (jmpBlk->bbWeight == BB_ZERO_WEIGHT) + { + jmpBlk->SetFlags(BBF_RUN_RARELY); + } - fgReplacePred(bDst, bSrc, jmpBlk); + weight_t weightDiff = (newEdge->edgeWeightMax() - newEdge->edgeWeightMin()); + weight_t slop = BasicBlock::GetSlopFraction(bSrc, bDst); + // + // If the [min/max] values for our edge weight is within the slop factor + // then we will set the BBF_PROF_WEIGHT flag for the block + // + if (weightDiff <= slop) + { + jmpBlk->SetFlags(BBF_PROF_WEIGHT); + } + } + else + { + // We set the bbWeight to the smaller of bSrc->bbWeight or bDst->bbWeight + if (bSrc->bbWeight < bDst->bbWeight) + { + jmpBlk->bbWeight = bSrc->bbWeight; + jmpBlk->CopyFlags(bSrc, BBF_RUN_RARELY); + } + else + { + jmpBlk->bbWeight = bDst->bbWeight; + jmpBlk->CopyFlags(bDst, BBF_RUN_RARELY); + } + } - JITDUMP("Added an unconditional jump to " FMT_BB " after block " FMT_BB "\n", - jmpBlk->GetTarget()->bbNum, bSrc->bbNum); - break; + fgReplacePred(bDst, bSrc, jmpBlk); - default: - noway_assert(!"Unexpected bbKind"); - break; - } + JITDUMP("Added an unconditional jump to " FMT_BB " after block " FMT_BB "\n", jmpBlk->GetTarget()->bbNum, + bSrc->bbNum); } else if (bSrc->KindIs(BBJ_ALWAYS) && bSrc->HasInitializedTarget() && bSrc->JumpsToNext()) { From c0e065e08360788387378dcb2b6e406e61e2e3bc Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 22 Dec 2023 18:08:30 -0500 Subject: [PATCH 7/7] More fgConnectFallThrough cleanup --- src/coreclr/jit/fgbasic.cpp | 114 +++++++++++++++++------------------- 1 file changed, 55 insertions(+), 59 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 24f1a282c6f603..e5b815f186478f 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -5516,7 +5516,7 @@ void Compiler::fgPrepareCallFinallyRetForRemoval(BasicBlock* block) // fgConnectFallThrough: fix flow from a block that previously had a fall through // // Arguments: -// bSrc - source of fall through (may be null?) +// bSrc - source of fall through // bDst - target of fall through // // Returns: @@ -5525,81 +5525,77 @@ void Compiler::fgPrepareCallFinallyRetForRemoval(BasicBlock* block) // BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst) { + assert(bSrc != nullptr); assert(fgPredsComputed); BasicBlock* jmpBlk = nullptr; - /* If bSrc is non-NULL */ + /* If bSrc falls through to a block that is not bDst, we will insert a jump to bDst */ - if (bSrc != nullptr) + if (bSrc->KindIs(BBJ_COND) && !bSrc->NextIs(bDst)) { - /* If bSrc falls through to a block that is not bDst, we will insert a jump to bDst */ + // Add a new block after bSrc which jumps to 'bDst' + jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst); + bSrc->SetFalseTarget(jmpBlk); + fgAddRefPred(jmpBlk, bSrc, fgGetPredForBlock(bDst, bSrc)); - if (bSrc->KindIs(BBJ_COND) && !bSrc->NextIs(bDst)) - { - // Add a new block after bSrc which jumps to 'bDst' - jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst); - bSrc->SetFalseTarget(jmpBlk); - fgAddRefPred(jmpBlk, bSrc, fgGetPredForBlock(bDst, bSrc)); + // Record the loop number in the new block + jmpBlk->bbNatLoopNum = bSrc->bbNatLoopNum; - // Record the loop number in the new block - jmpBlk->bbNatLoopNum = bSrc->bbNatLoopNum; + // When adding a new jmpBlk we will set the bbWeight and bbFlags + // + if (fgHaveValidEdgeWeights && fgHaveProfileWeights()) + { + FlowEdge* const newEdge = fgGetPredForBlock(jmpBlk, bSrc); - // When adding a new jmpBlk we will set the bbWeight and bbFlags - // - if (fgHaveValidEdgeWeights && fgHaveProfileWeights()) + jmpBlk->bbWeight = (newEdge->edgeWeightMin() + newEdge->edgeWeightMax()) / 2; + if (bSrc->bbWeight == BB_ZERO_WEIGHT) { - FlowEdge* const newEdge = fgGetPredForBlock(jmpBlk, bSrc); - - jmpBlk->bbWeight = (newEdge->edgeWeightMin() + newEdge->edgeWeightMax()) / 2; - if (bSrc->bbWeight == BB_ZERO_WEIGHT) - { - jmpBlk->bbWeight = BB_ZERO_WEIGHT; - } + jmpBlk->bbWeight = BB_ZERO_WEIGHT; + } - if (jmpBlk->bbWeight == BB_ZERO_WEIGHT) - { - jmpBlk->SetFlags(BBF_RUN_RARELY); - } + if (jmpBlk->bbWeight == BB_ZERO_WEIGHT) + { + jmpBlk->SetFlags(BBF_RUN_RARELY); + } - weight_t weightDiff = (newEdge->edgeWeightMax() - newEdge->edgeWeightMin()); - weight_t slop = BasicBlock::GetSlopFraction(bSrc, bDst); - // - // If the [min/max] values for our edge weight is within the slop factor - // then we will set the BBF_PROF_WEIGHT flag for the block - // - if (weightDiff <= slop) - { - jmpBlk->SetFlags(BBF_PROF_WEIGHT); - } + weight_t weightDiff = (newEdge->edgeWeightMax() - newEdge->edgeWeightMin()); + weight_t slop = BasicBlock::GetSlopFraction(bSrc, bDst); + // + // If the [min/max] values for our edge weight is within the slop factor + // then we will set the BBF_PROF_WEIGHT flag for the block + // + if (weightDiff <= slop) + { + jmpBlk->SetFlags(BBF_PROF_WEIGHT); + } + } + else + { + // We set the bbWeight to the smaller of bSrc->bbWeight or bDst->bbWeight + if (bSrc->bbWeight < bDst->bbWeight) + { + jmpBlk->bbWeight = bSrc->bbWeight; + jmpBlk->CopyFlags(bSrc, BBF_RUN_RARELY); } else { - // We set the bbWeight to the smaller of bSrc->bbWeight or bDst->bbWeight - if (bSrc->bbWeight < bDst->bbWeight) - { - jmpBlk->bbWeight = bSrc->bbWeight; - jmpBlk->CopyFlags(bSrc, BBF_RUN_RARELY); - } - else - { - jmpBlk->bbWeight = bDst->bbWeight; - jmpBlk->CopyFlags(bDst, BBF_RUN_RARELY); - } + jmpBlk->bbWeight = bDst->bbWeight; + jmpBlk->CopyFlags(bDst, BBF_RUN_RARELY); } + } - fgReplacePred(bDst, bSrc, jmpBlk); + fgReplacePred(bDst, bSrc, jmpBlk); - JITDUMP("Added an unconditional jump to " FMT_BB " after block " FMT_BB "\n", jmpBlk->GetTarget()->bbNum, - bSrc->bbNum); - } - else if (bSrc->KindIs(BBJ_ALWAYS) && bSrc->HasInitializedTarget() && bSrc->JumpsToNext()) - { - bSrc->SetFlags(BBF_NONE_QUIRK); - } - else if (bSrc->KindIs(BBJ_COND) && bSrc->NextIs(bDst)) - { - bSrc->SetFalseTarget(bDst); - } + JITDUMP("Added an unconditional jump to " FMT_BB " after block " FMT_BB "\n", jmpBlk->GetTarget()->bbNum, + bSrc->bbNum); + } + else if (bSrc->KindIs(BBJ_ALWAYS) && bSrc->HasInitializedTarget() && bSrc->JumpsToNext()) + { + bSrc->SetFlags(BBF_NONE_QUIRK); + } + else if (bSrc->KindIs(BBJ_COND) && bSrc->NextIs(bDst)) + { + bSrc->SetFalseTarget(bDst); } return jmpBlk;