From bae54ef4b83079ad3193eb46bd60191b043b3ed9 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 24 Jan 2024 12:07:09 -0500 Subject: [PATCH 01/12] Emit jumps for divergent false targets --- src/coreclr/jit/block.cpp | 10 ++++++---- src/coreclr/jit/block.h | 2 +- src/coreclr/jit/codegenarm.cpp | 7 +++++++ src/coreclr/jit/codegenarm64.cpp | 14 ++++++++++++++ src/coreclr/jit/codegencommon.cpp | 2 +- src/coreclr/jit/codegenlinear.cpp | 5 +++-- src/coreclr/jit/codegenloongarch64.cpp | 17 +++++++++++++++++ src/coreclr/jit/codegenriscv64.cpp | 7 +++++++ src/coreclr/jit/codegenxarch.cpp | 7 +++++++ 9 files changed, 63 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index c62ead94d2acfa..7ee142306cf131 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -301,18 +301,20 @@ bool BasicBlock::CanRemoveJumpToNext(Compiler* compiler) const } //------------------------------------------------------------------------ -// CanRemoveJumpToFalseTarget: determine if jump to false target can be omitted +// CanRemoveJumpToTarget: determine if jump to target can be omitted // // Arguments: +// target - true/false target of the BBJ_COND block // compiler - current compiler instance // // Returns: -// true if block is a BBJ_COND that can fall into its false target +// true if block is a BBJ_COND that can fall into target // -bool BasicBlock::CanRemoveJumpToFalseTarget(Compiler* compiler) const +bool BasicBlock::CanRemoveJumpToTarget(BasicBlock* target, Compiler* compiler) const { assert(KindIs(BBJ_COND)); - return NextIs(bbFalseTarget) && !hasAlign() && !compiler->fgInDifferentRegions(this, bbFalseTarget); + assert(TrueTargetIs(target) || FalseTargetIs(target)); + return NextIs(target) && !compiler->fgInDifferentRegions(this, target); } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index ab557e71565d36..981d48e3635a10 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -616,7 +616,7 @@ struct BasicBlock : private LIR::Range bool CanRemoveJumpToNext(Compiler* compiler) const; - bool CanRemoveJumpToFalseTarget(Compiler* compiler) const; + bool CanRemoveJumpToTarget(BasicBlock* target, Compiler* compiler) const; unsigned GetTargetOffs() const { diff --git a/src/coreclr/jit/codegenarm.cpp b/src/coreclr/jit/codegenarm.cpp index 28908f4392d798..4a8c08a89858e8 100644 --- a/src/coreclr/jit/codegenarm.cpp +++ b/src/coreclr/jit/codegenarm.cpp @@ -1317,6 +1317,13 @@ void CodeGen::genCodeForJTrue(GenTreeOp* jtrue) regNumber reg = genConsumeReg(op); inst_RV_RV(INS_tst, reg, reg, genActualType(op)); inst_JMP(EJ_ne, compiler->compCurBB->GetTrueTarget()); + + // If we cannot fall into the false target, emit a jump to it + BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget(); + if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler)) + { + inst_JMP(EJ_jmp, falseTarget); + } } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index d9681d12012eba..489aa6a744942d 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -4650,6 +4650,13 @@ void CodeGen::genCodeForJTrue(GenTreeOp* jtrue) GenTree* op = jtrue->gtGetOp1(); regNumber reg = genConsumeReg(op); GetEmitter()->emitIns_J_R(INS_cbnz, emitActualTypeSize(op), compiler->compCurBB->GetTrueTarget(), reg); + + // If we cannot fall into the false target, emit a jump to it + BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget(); + if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler)) + { + inst_JMP(EJ_jmp, falseTarget); + } } //------------------------------------------------------------------------ @@ -4877,6 +4884,13 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree) GetEmitter()->emitIns_J_R(ins, attr, compiler->compCurBB->GetTrueTarget(), reg); } + + // If we cannot fall into the false target, emit a jump to it + BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget(); + if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler)) + { + inst_JMP(EJ_jmp, falseTarget); + } } //--------------------------------------------------------------------- diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 94a44ecc2512db..133403e2f9d2bd 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -397,7 +397,7 @@ void CodeGen::genMarkLabelsForCodegen() block->GetTrueTarget()->SetFlags(BBF_HAS_LABEL); // If we need a jump to the false target, give it a label - if (!block->CanRemoveJumpToFalseTarget(compiler)) + if (!block->CanRemoveJumpToTarget(block->GetFalseTarget(), compiler)) { JITDUMP(" " FMT_BB " : branch target\n", block->GetFalseTarget()->bbNum); block->GetFalseTarget()->SetFlags(BBF_HAS_LABEL); diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 17d6f04bbdef55..67f4bb73c0eb18 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -2640,9 +2640,10 @@ void CodeGen::genCodeForJcc(GenTreeCC* jcc) inst_JCC(jcc->gtCondition, compiler->compCurBB->GetTrueTarget()); // If we cannot fall into the false target, emit a jump to it - if (!compiler->compCurBB->CanRemoveJumpToFalseTarget(compiler)) + BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget(); + if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler)) { - inst_JMP(EJ_jmp, compiler->compCurBB->GetFalseTarget()); + inst_JMP(EJ_jmp, falseTarget); } } diff --git a/src/coreclr/jit/codegenloongarch64.cpp b/src/coreclr/jit/codegenloongarch64.cpp index a0d325c4f29b6f..41266917205a52 100644 --- a/src/coreclr/jit/codegenloongarch64.cpp +++ b/src/coreclr/jit/codegenloongarch64.cpp @@ -4330,6 +4330,13 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree) assert(regs != 0); emit->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), regs); // 5-bits; + + // If we cannot fall into the false target, emit a jump to it + BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget(); + if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler)) + { + inst_JMP(EJ_jmp, falseTarget); + } } //--------------------------------------------------------------------- @@ -4884,6 +4891,16 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode) assert(jcc->gtCondition.Is(GenCondition::EQ, GenCondition::NE)); instruction ins = jcc->gtCondition.Is(GenCondition::EQ) ? INS_bceqz : INS_bcnez; emit->emitIns_J(ins, tgtBlock, (int)1 /* cc */); + + if (compiler->compCurBB->KindIs(BBJ_COND)) + { + // If we cannot fall into the false target, emit a jump to it + BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget(); + if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler)) + { + inst_JMP(EJ_jmp, falseTarget); + } + } } break; diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 73f82efa46d597..05f7a2c11f4432 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -4240,6 +4240,13 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree) assert(regs != 0); emit->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), regs); // 5-bits; + + // If we cannot fall into the false target, emit a jump to it + BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget(); + if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler)) + { + inst_JMP(EJ_jmp, falseTarget); + } } //--------------------------------------------------------------------- diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index b806ca975c5072..43c006291dd3e5 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -1466,6 +1466,13 @@ void CodeGen::genCodeForJTrue(GenTreeOp* jtrue) regNumber reg = genConsumeReg(op); inst_RV_RV(INS_test, reg, reg, genActualType(op)); inst_JMP(EJ_jne, compiler->compCurBB->GetTrueTarget()); + + // If we cannot fall into the false target, emit a jump to it + BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget(); + if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler)) + { + inst_JMP(EJ_jmp, falseTarget); + } } //------------------------------------------------------------------------ From d762c500263010f2842d38278ebca86648737350 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 24 Jan 2024 15:08:00 -0500 Subject: [PATCH 02/12] Allow false target to diverge after optOptimizeLayout --- src/coreclr/jit/block.cpp | 4 +- src/coreclr/jit/block.h | 3 - src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgbasic.cpp | 35 ++++---- src/coreclr/jit/fgdiagnostic.cpp | 7 -- src/coreclr/jit/fgopt.cpp | 110 ++++++++++++++------------ src/coreclr/jit/lower.cpp | 2 +- src/coreclr/jit/optimizebools.cpp | 13 ++- src/coreclr/jit/optimizer.cpp | 2 +- src/coreclr/jit/switchrecognition.cpp | 30 ++++++- 10 files changed, 120 insertions(+), 88 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 7ee142306cf131..b6f2f6e9f867a5 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -1171,7 +1171,7 @@ unsigned BasicBlock::NumSucc() const return 1; case BBJ_COND: - if (bbTarget == bbNext) + if (bbTrueTarget == bbFalseTarget) { return 1; } @@ -1296,7 +1296,7 @@ unsigned BasicBlock::NumSucc(Compiler* comp) return 1; case BBJ_COND: - if (bbTarget == bbNext) + if (bbTrueTarget == bbFalseTarget) { return 1; } diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 981d48e3635a10..70df96f7d21508 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -696,15 +696,12 @@ struct BasicBlock : private LIR::Range { assert(KindIs(BBJ_COND)); assert(bbFalseTarget != nullptr); - assert(target != nullptr); return (bbFalseTarget == target); } void SetCond(BasicBlock* trueTarget, BasicBlock* falseTarget) { assert(trueTarget != nullptr); - // TODO-NoFallThrough: Allow falseTarget to diverge from bbNext - assert(falseTarget == bbNext); bbKind = BBJ_COND; bbTrueTarget = trueTarget; bbFalseTarget = falseTarget; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index e823972ba864b7..a6dd4b3a1cb7dc 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5987,7 +5987,7 @@ class Compiler bool fgIsForwardBranch(BasicBlock* bJump, BasicBlock* bDest, BasicBlock* bSrc = nullptr); - bool fgUpdateFlowGraph(bool doTailDup = false, bool isPhase = false); + bool fgUpdateFlowGraph(bool doTailDup = false, bool isPhase = false, bool layoutFinalized = false); PhaseStatus fgUpdateFlowGraphPhase(); PhaseStatus fgDfsBlocksAndRemove(); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 879a7fa83edb19..9c031dab443159 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -5052,13 +5052,17 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ) if (curr->KindIs(BBJ_COND)) { - curr->SetFalseTarget(curr->Next()); - fgReplacePred(succ, curr, newBlock); if (curr->TrueTargetIs(succ)) { - // Now 'curr' jumps to newBlock curr->SetTrueTarget(newBlock); } + else + { + assert(curr->FalseTargetIs(succ)); + curr->SetFalseTarget(newBlock); + } + + fgReplacePred(succ, curr, newBlock); fgAddRefPred(newBlock, curr); } else if (curr->KindIs(BBJ_SWITCH)) @@ -5198,7 +5202,7 @@ void Compiler::fgUnlinkRange(BasicBlock* bBeg, BasicBlock* bEnd) // // Arguments: // block - the block to remove -// unreachable - the block to remove +// unreachable - indicates whether removal is because block is unreachable or empty // // Return Value: // The block after the block, or blocks, removed. @@ -5366,24 +5370,15 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable) break; case BBJ_COND: - /* The links for the direct predecessor case have already been updated above */ - if (!predBlock->TrueTargetIs(block)) + if (predBlock->TrueTargetIs(block)) { - break; + predBlock->SetTrueTarget(succBlock); } - /* Check if both sides of the BBJ_COND now jump to the same block */ - if (predBlock->FalseTargetIs(succBlock)) + if (predBlock->FalseTargetIs(block)) { - // Make sure we are replacing "block" with "succBlock" in predBlock->bbTarget. - noway_assert(predBlock->TrueTargetIs(block)); - predBlock->SetTrueTarget(succBlock); - fgRemoveConditionalJump(predBlock); - break; + predBlock->SetFalseTarget(succBlock); } - - noway_assert(predBlock->TrueTargetIs(block)); - predBlock->SetTrueTarget(succBlock); break; case BBJ_CALLFINALLY: @@ -5418,7 +5413,9 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable) break; case BBJ_COND: - bPrev->SetFalseTarget(block->Next()); + // block should not be a target anymore + assert(!bPrev->TrueTargetIs(block)); + assert(!bPrev->FalseTargetIs(block)); /* Check if both sides of the BBJ_COND now jump to the same block */ if (bPrev->TrueTargetIs(bPrev->GetFalseTarget())) @@ -5494,7 +5491,7 @@ 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->KindIs(BBJ_COND) && !bSrc->NextIs(bDst)) + if (bSrc->KindIs(BBJ_COND) && bSrc->FalseTargetIs(bDst) && !bSrc->NextIs(bDst)) { // Add a new block after bSrc which jumps to 'bDst' jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst); diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 92aac7828e1333..a491e4918398de 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -2920,13 +2920,6 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef maxBBNum = max(maxBBNum, block->bbNum); - // BBJ_COND's normal (false) jump target is expected to be the next block - // TODO-NoFallThrough: Allow bbFalseTarget to diverge from bbNext - if (block->KindIs(BBJ_COND)) - { - assert(block->NextIs(block->GetFalseTarget())); - } - // Check that all the successors have the current traversal stamp. Use the 'Compiler*' version of the // iterator, but not for BBJ_SWITCH: we don't want to end up calling GetDescriptorForSwitch(), which will // dynamically create the unique switch list. diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 39440510a60e7e..43a1dce92048f4 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -903,11 +903,7 @@ PhaseStatus Compiler::fgPostImportationCleanup() // bool Compiler::fgCanCompactBlocks(BasicBlock* block, BasicBlock* bNext) { - if ((block == nullptr) || (bNext == nullptr)) - { - return false; - } - + assert(block != nullptr); assert(block->NextIs(bNext)); if (!block->KindIs(BBJ_ALWAYS) || !block->TargetIs(bNext) || block->HasFlag(BBF_KEEP_BBJ_ALWAYS)) @@ -1039,6 +1035,13 @@ void Compiler::fgCompactBlocks(BasicBlock* block, BasicBlock* bNext) { BasicBlock* const predBlock = preds.Pop(); fgReplaceJumpTarget(predBlock, block, bNext); + + // TODO-NoFallThrough: Update fgReplaceJumpTarget to handle BBJ_COND false target case + if (predBlock->KindIs(BBJ_COND) && predBlock->FalseTargetIs(bNext)) + { + predBlock->SetFalseTarget(block); + fgAddRefPred(block, predBlock, fgRemoveRefPred(bNext, predBlock)); + } } } @@ -1682,7 +1685,16 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc break; case BBJ_COND: - block->SetTrueTarget(bDest->GetTarget()); + if (block->TrueTargetIs(bDest)) + { + assert(!block->FalseTargetIs(bDest)); + block->SetTrueTarget(bDest->GetTarget()); + } + else + { + assert(block->FalseTargetIs(bDest)); + block->SetFalseTarget(bDest->GetTarget()); + } break; default: @@ -4656,6 +4668,11 @@ bool Compiler::fgReorderBlocks(bool useProfile) noway_assert(condTest->gtOper == GT_JTRUE); condTest->AsOp()->gtOp1 = gtReverseCond(condTest->AsOp()->gtOp1); + BasicBlock* trueTarget = bPrev->GetTrueTarget(); + BasicBlock* falseTarget = bPrev->GetFalseTarget(); + bPrev->SetTrueTarget(falseTarget); + bPrev->SetFalseTarget(trueTarget); + // may need to rethread // if (fgNodeThreading == NodeThreading::AllTrees) @@ -4665,18 +4682,10 @@ bool Compiler::fgReorderBlocks(bool useProfile) fgSetStmtSeq(condTestStmt); } - if (bStart2 == nullptr) - { - /* Set the new jump dest for bPrev to the rarely run or uncommon block(s) */ - bPrev->SetTrueTarget(bStart); - } - else + if (bStart2 != nullptr) { noway_assert(insertAfterBlk == bPrev); noway_assert(insertAfterBlk->NextIs(block)); - - /* Set the new jump dest for bPrev to the rarely run or uncommon block(s) */ - bPrev->SetTrueTarget(block); } } @@ -4808,6 +4817,7 @@ PhaseStatus Compiler::fgUpdateFlowGraphPhase() // Arguments: // doTailDuplication - true to attempt tail duplication optimization // isPhase - true if being run as the only thing in a phase +// layoutFinalized - true if being run after block reordering // // Returns: true if the flowgraph has been modified // @@ -4815,7 +4825,7 @@ PhaseStatus Compiler::fgUpdateFlowGraphPhase() // Debuggable code and Min Optimization JIT also introduces basic blocks // but we do not optimize those! // -bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) +bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPhase /* = false */, bool layoutFinalized /* = false */) { #ifdef DEBUG if (verbose && !isPhase) @@ -4854,6 +4864,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) BasicBlock* bPrev = nullptr; // the previous non-worthless block BasicBlock* bNext; // the successor of the current block BasicBlock* bDest; // the jump target of the current block + BasicBlock* bFalseDest; // the false target of the current block (if it is a BBJ_COND) for (block = fgFirstBB; block != nullptr; block = block->Next()) { @@ -4887,8 +4898,9 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) REPEAT:; - bNext = block->Next(); - bDest = nullptr; + bNext = block->Next(); + bDest = nullptr; + bFalseDest = nullptr; if (block->KindIs(BBJ_ALWAYS)) { @@ -4923,25 +4935,23 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) } else if (block->KindIs(BBJ_COND)) { - bDest = block->GetTrueTarget(); - if (bDest == bNext) + bDest = block->GetTrueTarget(); + bFalseDest = block->GetFalseTarget(); + if (bDest == bFalseDest) { - // TODO-NoFallThrough: Fix above condition once bbFalseTarget can diverge from bbNext - assert(block->FalseTargetIs(bNext)); - if (fgOptimizeBranchToNext(block, bNext, bPrev)) - { - change = true; - modified = true; - bDest = nullptr; - } + fgRemoveConditionalJump(block); + assert(block->KindIs(BBJ_ALWAYS)); + change = true; + modified = true; + bFalseDest = nullptr; } } if (bDest != nullptr) { // Do we have a JUMP to an empty unconditional JUMP block? - if (bDest->isEmpty() && bDest->KindIs(BBJ_ALWAYS) && - !bDest->TargetIs(bDest)) // special case for self jumps + if (bDest->KindIs(BBJ_ALWAYS) && !bDest->TargetIs(bDest) && // special case for self jumps + bDest->isEmpty()) { // TODO: Allow optimizing branches to blocks that jump to the next block const bool optimizeBranch = !bDest->JumpsToNext() || !bDest->HasFlag(BBF_NONE_QUIRK); @@ -4953,6 +4963,17 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) } } + if (layoutFinalized && (bFalseDest != nullptr) && bFalseDest->KindIs(BBJ_ALWAYS) && !bFalseDest->TargetIs(bFalseDest) && // special case for self jumps + bFalseDest->isEmpty()) + { + if (fgOptimizeBranchToEmptyUnconditional(block, bFalseDest)) + { + change = true; + modified = true; + goto REPEAT; + } + } + // Check for cases where reversing the branch condition may enable // other flow opts. // @@ -4962,17 +4983,16 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) // bNext's jump target has a join. // if (block->KindIs(BBJ_COND) && // block is a BBJ_COND block - (bNext->bbRefs == 1) && // No other block jumps to bNext - bNext->KindIs(BBJ_ALWAYS) && // The next block is a BBJ_ALWAYS block + (bFalseDest == bNext) && // false target is the next block + (bNext->bbRefs == 1) && // no other block jumps to bNext + bNext->KindIs(BBJ_ALWAYS) && // the next block is a BBJ_ALWAYS block !bNext->JumpsToNext() && // and it doesn't jump to the next block (we might compact them) bNext->isEmpty() && // and it is an empty block !bNext->TargetIs(bNext) && // special case for self jumps !bDest->IsFirstColdBlock(this) && !fgInDifferentRegions(block, bDest)) // do not cross hot/cold sections { - // bbFalseTarget cannot be null assert(block->FalseTargetIs(bNext)); - assert(bNext != nullptr); // case (a) // @@ -5046,15 +5066,6 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) JITDUMP("\nMoving " FMT_BB " after " FMT_BB " to enable reversal\n", bDest->bbNum, bNext->bbNum); - // If bDest can fall through we'll need to create a jump - // block after it too. Remember where to jump to. - // - BasicBlock* const bDestNext = bDest->Next(); - - // Once bbFalseTarget and bbNext can diverge, this assert will hit - // TODO-NoFallThrough: Remove fall-through for BBJ_COND below - assert(!bDest->KindIs(BBJ_COND) || bDest->FalseTargetIs(bDestNext)); - // Move bDest // if (ehIsBlockEHLast(bDest)) @@ -5072,15 +5083,16 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) // Add fall through fixup block, if needed. // - if (bDest->KindIs(BBJ_COND)) + if (!layoutFinalized && bDest->KindIs(BBJ_COND) && !bDest->NextIs(bDest->GetFalseTarget())) { - BasicBlock* const bFixup = fgNewBBafter(BBJ_ALWAYS, bDest, true, bDestNext); + BasicBlock* const bDestFalseTarget = bDest->GetFalseTarget(); + BasicBlock* const bFixup = fgNewBBafter(BBJ_ALWAYS, bDest, true, bDestFalseTarget); bDest->SetFalseTarget(bFixup); - bFixup->inheritWeight(bDestNext); + bFixup->inheritWeight(bDestFalseTarget); - fgRemoveRefPred(bDestNext, bDest); + fgRemoveRefPred(bDestFalseTarget, bDest); fgAddRefPred(bFixup, bDest); - fgAddRefPred(bDestNext, bFixup); + fgAddRefPred(bDestFalseTarget, bFixup); } } } @@ -5243,7 +5255,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) switch (block->GetKind()) { case BBJ_COND: - if (block->TrueTargetIs(block)) + if (block->TrueTargetIs(block) || block->FalseTargetIs(block)) { fgRemoveBlock(block, /* unreachable */ true); diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index f1142bcbd215b1..0555a016b5b23c 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7200,7 +7200,7 @@ PhaseStatus Lowering::DoPhase() // local var liveness can delete code, which may create empty blocks if (comp->opts.OptimizationEnabled()) { - bool modified = comp->fgUpdateFlowGraph(); + bool modified = comp->fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false, /* layoutFinalized */ true); modified |= comp->fgRemoveDeadBlocks(); if (modified) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 7cca29bc7dbfef..f9616636681b5c 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -751,7 +751,7 @@ bool OptBoolsDsc::optOptimizeRangeTests() // BasicBlock* notInRangeBb = m_b1->GetTrueTarget(); BasicBlock* inRangeBb; - if (notInRangeBb == m_b2->GetTrueTarget()) + if (m_b2->TrueTargetIs(notInRangeBb)) { // Shape 1: both conditions jump to NotInRange // @@ -765,7 +765,7 @@ bool OptBoolsDsc::optOptimizeRangeTests() // ... inRangeBb = m_b2->GetFalseTarget(); } - else if (notInRangeBb == m_b2->GetFalseTarget()) + else if (m_b2->FalseTargetIs(notInRangeBb)) { // Shape 2: 2nd block jumps to InRange // @@ -807,11 +807,16 @@ bool OptBoolsDsc::optOptimizeRangeTests() return false; } + // Re-direct firstBlock to jump to inRangeBb m_comp->fgAddRefPred(inRangeBb, m_b1); if (!cmp2IsReversed) { - // Re-direct firstBlock to jump to inRangeBb m_b1->SetTrueTarget(inRangeBb); + m_b1->SetFalseTarget(notInRangeBb); + } + else + { + m_b1->SetFalseTarget(inRangeBb); } // Remove the 2nd condition block as we no longer need it @@ -1015,7 +1020,7 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() m_b2->CopyFlags(m_b1, BBF_COPY_PROPAGATE); // Join the two blocks. This is done now to ensure that additional conditions can be chained. - if (m_comp->fgCanCompactBlocks(m_b1, m_b2)) + if (m_b1->NextIs(m_b2) && m_comp->fgCanCompactBlocks(m_b1, m_b2)) { m_comp->fgCompactBlocks(m_b1, m_b2); } diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index d124ac28fd865f..916fa9f78e2f6c 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2438,7 +2438,7 @@ PhaseStatus Compiler::optOptimizeLayout() fgUpdateFlowGraph(/* doTailDuplication */ false); fgReorderBlocks(/* useProfile */ true); - fgUpdateFlowGraph(); + fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false, /* layoutFinalized */ true); // fgReorderBlocks can cause IR changes even if it does not modify // the flow graph. It calls gtPrepareCost which can cause operand swapping. diff --git a/src/coreclr/jit/switchrecognition.cpp b/src/coreclr/jit/switchrecognition.cpp index 7ab33d07c3c34b..fa6abd0f23e8bb 100644 --- a/src/coreclr/jit/switchrecognition.cpp +++ b/src/coreclr/jit/switchrecognition.cpp @@ -349,6 +349,31 @@ bool Compiler::optSwitchConvert(BasicBlock* firstBlock, int testsCount, ssize_t* assert((jumpCount > 0) && (jumpCount <= SWITCH_MAX_DISTANCE + 1)); const auto jmpTab = new (this, CMK_BasicBlock) BasicBlock*[jumpCount + 1 /*default case*/]; + // Quirk: lastBlock's false target may have diverged from bbNext. If the false target is behind firstBlock, + // we may create a cycle in the BasicBlock list by setting firstBlock->bbNext to it. + // Add a new BBJ_ALWAYS to the false target to avoid this. + // (We only need this if the false target is behind firstBlock, + // but it's cheaper to just check if the false target has diverged) + // TODO-NoFallThrough: Revisit this quirk? + bool skipPredRemoval = false; + if (!lastBlock->FalseTargetIs(lastBlock->Next())) + { + if (isReversed) + { + assert(lastBlock->FalseTargetIs(blockIfTrue)); + fgRemoveRefPred(blockIfTrue, firstBlock); + blockIfTrue = fgNewBBafter(BBJ_ALWAYS, firstBlock, true, blockIfTrue); + fgAddRefPred(blockIfTrue->GetTarget(), blockIfTrue); + skipPredRemoval = true; + } + else + { + assert(lastBlock->FalseTargetIs(blockIfFalse)); + blockIfFalse = fgNewBBafter(BBJ_ALWAYS, firstBlock, true, blockIfFalse); + fgAddRefPred(blockIfFalse->GetTarget(), blockIfFalse); + } + } + fgHasSwitch = true; firstBlock->GetSwitchTargets()->bbsCount = jumpCount + 1; firstBlock->GetSwitchTargets()->bbsHasDefault = true; @@ -368,7 +393,10 @@ bool Compiler::optSwitchConvert(BasicBlock* firstBlock, int testsCount, ssize_t* } // Unlink blockIfTrue from firstBlock, we're going to link it again in the loop below. - fgRemoveRefPred(blockIfTrue, firstBlock); + if (!skipPredRemoval) + { + fgRemoveRefPred(blockIfTrue, firstBlock); + } for (unsigned i = 0; i < jumpCount; i++) { From b5a8837a5f6abdb93420bcb82b8af929aecbdb99 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 24 Jan 2024 16:36:17 -0500 Subject: [PATCH 03/12] Strip out layoutFinalized --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgopt.cpp | 16 ++-------------- src/coreclr/jit/lower.cpp | 2 +- src/coreclr/jit/optimizer.cpp | 2 +- 4 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a6dd4b3a1cb7dc..e823972ba864b7 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5987,7 +5987,7 @@ class Compiler bool fgIsForwardBranch(BasicBlock* bJump, BasicBlock* bDest, BasicBlock* bSrc = nullptr); - bool fgUpdateFlowGraph(bool doTailDup = false, bool isPhase = false, bool layoutFinalized = false); + bool fgUpdateFlowGraph(bool doTailDup = false, bool isPhase = false); PhaseStatus fgUpdateFlowGraphPhase(); PhaseStatus fgDfsBlocksAndRemove(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 43a1dce92048f4..ffaf29132754de 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4817,7 +4817,6 @@ PhaseStatus Compiler::fgUpdateFlowGraphPhase() // Arguments: // doTailDuplication - true to attempt tail duplication optimization // isPhase - true if being run as the only thing in a phase -// layoutFinalized - true if being run after block reordering // // Returns: true if the flowgraph has been modified // @@ -4825,7 +4824,7 @@ PhaseStatus Compiler::fgUpdateFlowGraphPhase() // Debuggable code and Min Optimization JIT also introduces basic blocks // but we do not optimize those! // -bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPhase /* = false */, bool layoutFinalized /* = false */) +bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPhase /* = false */) { #ifdef DEBUG if (verbose && !isPhase) @@ -4963,17 +4962,6 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh } } - if (layoutFinalized && (bFalseDest != nullptr) && bFalseDest->KindIs(BBJ_ALWAYS) && !bFalseDest->TargetIs(bFalseDest) && // special case for self jumps - bFalseDest->isEmpty()) - { - if (fgOptimizeBranchToEmptyUnconditional(block, bFalseDest)) - { - change = true; - modified = true; - goto REPEAT; - } - } - // Check for cases where reversing the branch condition may enable // other flow opts. // @@ -5083,7 +5071,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh // Add fall through fixup block, if needed. // - if (!layoutFinalized && bDest->KindIs(BBJ_COND) && !bDest->NextIs(bDest->GetFalseTarget())) + if (bDest->KindIs(BBJ_COND) && !bDest->NextIs(bDest->GetFalseTarget())) { BasicBlock* const bDestFalseTarget = bDest->GetFalseTarget(); BasicBlock* const bFixup = fgNewBBafter(BBJ_ALWAYS, bDest, true, bDestFalseTarget); diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 0555a016b5b23c..e96ebcf9c3a66d 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7200,7 +7200,7 @@ PhaseStatus Lowering::DoPhase() // local var liveness can delete code, which may create empty blocks if (comp->opts.OptimizationEnabled()) { - bool modified = comp->fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false, /* layoutFinalized */ true); + bool modified = comp->fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false); modified |= comp->fgRemoveDeadBlocks(); if (modified) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 916fa9f78e2f6c..5c7609d6adb62c 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2438,7 +2438,7 @@ PhaseStatus Compiler::optOptimizeLayout() fgUpdateFlowGraph(/* doTailDuplication */ false); fgReorderBlocks(/* useProfile */ true); - fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false, /* layoutFinalized */ true); + fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false); // fgReorderBlocks can cause IR changes even if it does not modify // the flow graph. It calls gtPrepareCost which can cause operand swapping. From 3cc753fe77cafe8825e703da3fdc2ebcad008429 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 24 Jan 2024 16:41:33 -0500 Subject: [PATCH 04/12] Add fallthrough fix before block layout --- src/coreclr/jit/optimizer.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 5c7609d6adb62c..e4255ecfe0d875 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2437,6 +2437,20 @@ PhaseStatus Compiler::optOptimizeLayout() noway_assert(opts.OptimizationEnabled()); fgUpdateFlowGraph(/* doTailDuplication */ false); + + for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) + { + if (block->KindIs(BBJ_COND) && !block->NextIs(block->GetFalseTarget())) + { + BasicBlock* jmpBlk = fgConnectFallThrough(block, block->GetFalseTarget()); + assert(jmpBlk != nullptr); + assert(block->NextIs(jmpBlk)); + + // Skip next block + block = jmpBlk; + } + } + fgReorderBlocks(/* useProfile */ true); fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false); From a0040cc5dd345de65cd5374e3eff7bb533c8cd08 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 24 Jan 2024 17:37:05 -0500 Subject: [PATCH 05/12] Remove almost all fgConnectFallThrough calls --- src/coreclr/jit/block.cpp | 6 +- src/coreclr/jit/block.h | 1 - src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgbasic.cpp | 42 +++++-- src/coreclr/jit/fgopt.cpp | 25 ++-- src/coreclr/jit/fgprofile.cpp | 8 -- src/coreclr/jit/flowgraph.cpp | 4 +- src/coreclr/jit/jiteh.cpp | 7 -- src/coreclr/jit/loopcloning.cpp | 52 +------- src/coreclr/jit/optimizer.cpp | 156 +++++++----------------- src/coreclr/jit/redundantbranchopts.cpp | 3 +- 11 files changed, 99 insertions(+), 207 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index b6f2f6e9f867a5..99d1f3c54cafa1 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -843,8 +843,7 @@ void BasicBlock::CopyTarget(Compiler* compiler, const BasicBlock* from) SetEhf(new (compiler, CMK_BasicBlock) BBehfDesc(compiler, from->GetEhfTargets())); break; case BBJ_COND: - // TODO-NoFallThrough: Copy false target, too? - SetCond(from->GetTrueTarget(), Next()); + SetCond(from->GetTrueTarget(), from->GetFalseTarget()); break; case BBJ_ALWAYS: SetKindAndTarget(from->GetKind(), from->GetTarget()); @@ -885,8 +884,7 @@ void BasicBlock::TransferTarget(BasicBlock* from) from->bbEhfTargets = nullptr; // Make sure nobody uses the descriptor after this. break; case BBJ_COND: - // TODO-NoFallThrough: Copy false target, too? - SetCond(from->GetTrueTarget(), Next()); + SetCond(from->GetTrueTarget(), from->GetFalseTarget()); break; case BBJ_ALWAYS: SetKindAndTarget(from->GetKind(), from->GetTarget()); diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 70df96f7d21508..e42bab3b1b3576 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -669,7 +669,6 @@ struct BasicBlock : private LIR::Range { assert(KindIs(BBJ_COND)); assert(bbTrueTarget != nullptr); - assert(target != nullptr); return (bbTrueTarget == target); } diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index e823972ba864b7..cc6cc5bbf1aae8 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6775,7 +6775,7 @@ class Compiler bool optCompactLoop(FlowGraphNaturalLoop* loop); BasicBlock* optFindLoopCompactionInsertionPoint(FlowGraphNaturalLoop* loop, BasicBlock* top); BasicBlock* optTryAdvanceLoopCompactionInsertionPoint(FlowGraphNaturalLoop* loop, BasicBlock* insertionPoint, BasicBlock* top, BasicBlock* bottom); - bool optLoopCompactionFixupFallThrough(BasicBlock* block, BasicBlock* oldNext, BasicBlock* newNext); + bool optLoopCompactionFixupFallThrough(BasicBlock* block, BasicBlock* newNext); bool optCreatePreheader(FlowGraphNaturalLoop* loop); void optSetPreheaderWeight(FlowGraphNaturalLoop* loop, BasicBlock* preheader); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 9c031dab443159..25f56fdc88ce6b 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -693,15 +693,34 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* newTarget, Bas break; case BBJ_COND: + { + FlowEdge* oldEdge = fgGetPredForBlock(oldTarget, block); + assert(oldEdge != nullptr); - // Functionally equivalent to above if (block->TrueTargetIs(oldTarget)) { - block->SetTrueTarget(newTarget); - fgRemoveRefPred(oldTarget, block); - fgAddRefPred(newTarget, block); + if (block->FalseTargetIs(oldTarget)) + { + assert(oldEdge->getDupCount() == 2); + fgRemoveConditionalJump(block); + assert(block->KindIs(BBJ_ALWAYS)); + block->SetTarget(newTarget); + } + else + { + block->SetTrueTarget(newTarget); + } + } + else + { + assert(block->FalseTargetIs(oldTarget)); + block->SetFalseTarget(newTarget); } + + assert(oldEdge->getDupCount() == 1); + fgAddRefPred(newTarget, block, fgRemoveRefPred(oldTarget, block)); break; + } case BBJ_SWITCH: { @@ -6067,11 +6086,15 @@ BasicBlock* Compiler::fgRelocateEHRange(unsigned regionIndex, FG_RELOCATE_TYPE r // We have decided to insert the block(s) after fgLastBlock fgMoveBlocksAfter(bStart, bLast, insertAfterBlk); - // If bPrev falls through, we will insert a jump to block - fgConnectFallThrough(bPrev, bStart); + if (bPrev->KindIs(BBJ_ALWAYS) && bPrev->JumpsToNext()) + { + bPrev->SetFlags(BBF_NONE_QUIRK); + } - // If bLast falls through, we will insert a jump to bNext - fgConnectFallThrough(bLast, bNext); + if (bLast->KindIs(BBJ_ALWAYS) && bLast->JumpsToNext()) + { + bLast->SetFlags(BBF_NONE_QUIRK); + } #endif // !FEATURE_EH_FUNCLETS @@ -6962,9 +6985,6 @@ BasicBlock* Compiler::fgNewBBinRegionWorker(BBKinds jumpKind, } } - /* If afterBlk falls through, we insert a jump around newBlk */ - fgConnectFallThrough(afterBlk, newBlk->Next()); - #ifdef DEBUG fgVerifyHandlerTab(); #endif diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index ffaf29132754de..204e58f4435931 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1035,13 +1035,6 @@ void Compiler::fgCompactBlocks(BasicBlock* block, BasicBlock* bNext) { BasicBlock* const predBlock = preds.Pop(); fgReplaceJumpTarget(predBlock, block, bNext); - - // TODO-NoFallThrough: Update fgReplaceJumpTarget to handle BBJ_COND false target case - if (predBlock->KindIs(BBJ_COND) && predBlock->FalseTargetIs(bNext)) - { - predBlock->SetFalseTarget(block); - fgAddRefPred(block, predBlock, fgRemoveRefPred(bNext, predBlock)); - } } } @@ -3630,6 +3623,23 @@ bool Compiler::fgReorderBlocks(bool useProfile) } } + // If we will be reordering blocks, re-establish implicit fallthrough for BBJ_COND blocks + if (useProfile) + { + for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) + { + if (block->KindIs(BBJ_COND) && !block->NextIs(block->GetFalseTarget())) + { + BasicBlock* jmpBlk = fgConnectFallThrough(block, block->GetFalseTarget()); + assert(jmpBlk != nullptr); + assert(block->NextIs(jmpBlk)); + + // Skip next block + block = jmpBlk; + } + } + } + #ifdef DEBUG if (verbose) { @@ -3689,6 +3699,7 @@ bool Compiler::fgReorderBlocks(bool useProfile) } else if (bPrev->KindIs(BBJ_COND)) { + assert(bPrev->FalseTargetIs(block)); bDest = bPrev->GetTrueTarget(); forwardBranch = fgIsForwardBranch(bPrev, bDest); backwardBranch = !forwardBranch; diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index a4d9c217224715..504449a7c178a1 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -521,14 +521,6 @@ void BlockCountInstrumentor::RelocateProbes() // Redirect any jumps // m_comp->fgReplaceJumpTarget(pred, intermediary, block); - - // Handle case where we had a fall through critical edge - // - if (pred->NextIs(intermediary)) - { - m_comp->fgRemoveRefPred(pred, block); - m_comp->fgAddRefPred(intermediary, block); - } } } } diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index d475ff7ec1c2b4..56ad882b820f69 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4373,9 +4373,9 @@ FlowGraphNaturalLoops* FlowGraphNaturalLoops::Find(const FlowGraphDfsTree* dfsTr for (FlowEdge* const predEdge : loop->m_header->PredEdges()) { BasicBlock* predBlock = predEdge->getSourceBlock(); - if (dfsTree->Contains(predBlock) && !dfsTree->IsAncestor(header, predEdge->getSourceBlock())) + if (dfsTree->Contains(predBlock) && !dfsTree->IsAncestor(header, predBlock)) { - JITDUMP(FMT_BB " -> " FMT_BB " is an entry edge\n", predEdge->getSourceBlock()->bbNum, + JITDUMP(FMT_BB " -> " FMT_BB " is an entry edge\n", predBlock->bbNum, loop->m_header->bbNum); loop->m_entryEdges.push_back(predEdge); } diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index f1bdeeb22265fd..7242380cb605b7 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -2256,13 +2256,6 @@ bool Compiler::fgNormalizeEHCase2() // fgReplaceJumpTarget(predBlock, newTryStart, insertBeforeBlk); - if (predBlock->NextIs(newTryStart) && predBlock->KindIs(BBJ_COND)) - { - predBlock->SetFalseTarget(newTryStart); - fgRemoveRefPred(insertBeforeBlk, predBlock); - fgAddRefPred(newTryStart, predBlock); - } - JITDUMP("Redirect " FMT_BB " target from " FMT_BB " to " FMT_BB ".\n", predBlock->bbNum, insertBeforeBlk->bbNum, newTryStart->bbNum); } diff --git a/src/coreclr/jit/loopcloning.cpp b/src/coreclr/jit/loopcloning.cpp index 730eb91738f670..ed6fddaba85c34 100644 --- a/src/coreclr/jit/loopcloning.cpp +++ b/src/coreclr/jit/loopcloning.cpp @@ -2043,25 +2043,6 @@ void Compiler::optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* contex // bottomNext BasicBlock* bottom = loop->GetLexicallyBottomMostBlock(); BasicBlock* newPred = bottom; - if (bottom->KindIs(BBJ_COND)) - { - // 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); - bottomRedirBlk->bbWeight = bottomRedirBlk->isRunRarely() ? BB_ZERO_WEIGHT : ambientWeight; - - bottom->SetFalseTarget(bottomRedirBlk); - fgAddRefPred(bottomRedirBlk, bottom); - JITDUMP("Adding " FMT_BB " -> " FMT_BB "\n", bottom->bbNum, bottomRedirBlk->bbNum); - fgReplacePred(bottomNext, bottom, bottomRedirBlk); - JITDUMP("Replace " FMT_BB " -> " FMT_BB " with " FMT_BB " -> " FMT_BB "\n", bottom->bbNum, bottomNext->bbNum, - bottomRedirBlk->bbNum, bottomNext->bbNum); - - newPred = bottomRedirBlk; - } // Create a new preheader for the slow loop immediately before the slow // loop itself. All failed conditions will branch to the slow preheader. @@ -2112,37 +2093,6 @@ void Compiler::optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* contex newPred = newBlk; blockMap->Set(blk, newBlk); - // If the block falls through to a block outside the loop then we may - // need to insert a new block to redirect. - // Skip this for the bottom block; we duplicate the slow loop such that - // the bottom block will fall through to the bottom's original next. - if ((blk != bottom) && blk->bbFallsThrough() && !loop->ContainsBlock(blk->Next())) - { - if (blk->KindIs(BBJ_COND)) - { - BasicBlock* targetBlk = blk->GetFalseTarget(); - assert(blk->NextIs(targetBlk)); - - // Need to insert a block. - BasicBlock* newRedirBlk = fgNewBBafter(BBJ_ALWAYS, newPred, /* extendRegion */ true, targetBlk); - newRedirBlk->copyEHRegion(newPred); - newRedirBlk->bbWeight = blk->Next()->bbWeight; - newRedirBlk->CopyFlags(blk->Next(), (BBF_RUN_RARELY | BBF_PROF_WEIGHT)); - newRedirBlk->scaleBBWeight(slowPathWeightScaleFactor); - - JITDUMP(FMT_BB " falls through to " FMT_BB "; inserted redirection block " FMT_BB "\n", blk->bbNum, - blk->Next()->bbNum, newRedirBlk->bbNum); - // This block isn't part of the loop, so below loop won't add - // refs for it. - fgAddRefPred(targetBlk, newRedirBlk); - newPred = newRedirBlk; - } - else - { - assert(!"Cannot handle fallthrough"); - } - } - return BasicBlockVisit::Continue; }); @@ -2165,7 +2115,7 @@ void Compiler::optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* contex // Now redirect the new block according to "blockMap". optRedirectBlock(newblk, blockMap); - // Add predecessor edges for the new successors, as well as the fall-through paths. + // Add predecessor edges for the new successors. switch (newblk->GetKind()) { case BBJ_ALWAYS: diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index e4255ecfe0d875..976e17a6859cd7 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -560,7 +560,6 @@ void Compiler::optCheckPreds() // predOption - specifies how to update the pred lists // // Notes: -// Fall-through successors are assumed correct and are not modified. // Pred lists for successors of `blk` may be changed, depending on `predOption`. // void Compiler::optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap, RedirectBlockOption predOption) @@ -568,11 +567,6 @@ void Compiler::optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap, R const bool updatePreds = (predOption == RedirectBlockOption::UpdatePredLists); const bool addPreds = (predOption == RedirectBlockOption::AddToPredLists); - if (addPreds && blk->bbFallsThrough()) - { - fgAddRefPred(blk->Next(), blk); - } - BasicBlock* newJumpDest = nullptr; switch (blk->GetKind()) @@ -585,9 +579,15 @@ void Compiler::optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap, R // These have no jump destination to update. break; + case BBJ_CALLFINALLY: + if (addPreds && blk->bbFallsThrough()) + { + fgAddRefPred(blk->Next(), blk); + } + + FALLTHROUGH; case BBJ_ALWAYS: case BBJ_LEAVE: - case BBJ_CALLFINALLY: case BBJ_CALLFINALLYRET: // All of these have a single jump destination to update. if (redirectMap->Lookup(blk->GetTarget(), &newJumpDest)) @@ -626,6 +626,24 @@ void Compiler::optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap, R { fgAddRefPred(blk->GetTrueTarget(), blk); } + + // Update jump taken when condition is false + if (redirectMap->Lookup(blk->GetFalseTarget(), &newJumpDest)) + { + if (updatePreds) + { + fgRemoveRefPred(blk->GetFalseTarget(), blk); + } + blk->SetFalseTarget(newJumpDest); + if (updatePreds || addPreds) + { + fgAddRefPred(newJumpDest, blk); + } + } + else if (addPreds) + { + fgAddRefPred(blk->GetFalseTarget(), blk); + } break; case BBJ_EHFINALLYRET: @@ -1586,28 +1604,6 @@ bool Compiler::optTryUnrollLoop(FlowGraphNaturalLoop* loop, bool* changedIR) BasicBlock* exit = loop->ContainsBlock(exiting->GetTrueTarget()) ? exiting->GetFalseTarget() : exiting->GetTrueTarget(); - // If the bottom block falls out of the loop, then insert an - // explicit block to branch around the unrolled iterations we are - // going to create. - if (bottom->KindIs(BBJ_COND)) - { - // 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 unrolled loop\n"); - BasicBlock* bottomRedirBlk = fgNewBBafter(BBJ_ALWAYS, bottom, /*extendRegion*/ true, bottomNext); - JITDUMP("Adding " FMT_BB " after " FMT_BB "\n", bottomRedirBlk->bbNum, bottom->bbNum); - - bottom->SetFalseTarget(bottomRedirBlk); - fgAddRefPred(bottomRedirBlk, bottom); - JITDUMP("Adding " FMT_BB " -> " FMT_BB "\n", bottom->bbNum, bottomRedirBlk->bbNum); - fgReplacePred(bottomNext, bottom, bottomRedirBlk); - JITDUMP("Replace " FMT_BB " -> " FMT_BB " with " FMT_BB " -> " FMT_BB "\n", bottom->bbNum, bottomNext->bbNum, - bottomRedirBlk->bbNum, bottomNext->bbNum); - - insertAfter = bottomRedirBlk; - } - for (int lval = lbeg; iterToUnroll > 0; iterToUnroll--) { BasicBlock* testBlock = nullptr; @@ -2437,20 +2433,6 @@ PhaseStatus Compiler::optOptimizeLayout() noway_assert(opts.OptimizationEnabled()); fgUpdateFlowGraph(/* doTailDuplication */ false); - - for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) - { - if (block->KindIs(BBJ_COND) && !block->NextIs(block->GetFalseTarget())) - { - BasicBlock* jmpBlk = fgConnectFallThrough(block, block->GetFalseTarget()); - assert(jmpBlk != nullptr); - assert(block->NextIs(jmpBlk)); - - // Skip next block - block = jmpBlk; - } - } - fgReorderBlocks(/* useProfile */ true); fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false); @@ -2833,10 +2815,10 @@ bool Compiler::optCompactLoop(FlowGraphNaturalLoop* loop) ehUpdateLastBlocks(insertionPoint, lastNonLoopBlock); // Apply any adjustments needed for fallthrough at the boundaries of the moved region. - changedFlowGraph |= optLoopCompactionFixupFallThrough(insertionPoint, moveBefore, cur); - changedFlowGraph |= optLoopCompactionFixupFallThrough(lastNonLoopBlock, nextLoopBlock, moveBefore); + changedFlowGraph |= optLoopCompactionFixupFallThrough(insertionPoint, cur); + changedFlowGraph |= optLoopCompactionFixupFallThrough(lastNonLoopBlock, moveBefore); // Also apply any adjustments needed where the blocks were snipped out of the loop. - changedFlowGraph |= optLoopCompactionFixupFallThrough(previous, cur, nextLoopBlock); + changedFlowGraph |= optLoopCompactionFixupFallThrough(previous, nextLoopBlock); // Update insertionPoint for the next insertion. insertionPoint = lastNonLoopBlock; @@ -2952,47 +2934,36 @@ BasicBlock* Compiler::optTryAdvanceLoopCompactionInsertionPoint(FlowGraphNatural // // Parameters: // block - Block that may have fallthrough -// oldNext - The old block that was the fallthrough block // newNext - The new block that was the fallthrough block // // Returns: // True if the flow graph was changed by this function. // -bool Compiler::optLoopCompactionFixupFallThrough(BasicBlock* block, BasicBlock* oldNext, BasicBlock* newNext) +bool Compiler::optLoopCompactionFixupFallThrough(BasicBlock* block, BasicBlock* newNext) { + assert(block->NextIs(newNext)); bool changed = false; - if (block->bbFallsThrough()) + if (block->KindIs(BBJ_COND) && block->TrueTargetIs(newNext)) { - // Need to reconnect the flow from `block` to `oldNext`. + // Reverse the jump condition + GenTree* test = block->lastNode(); + noway_assert(test->OperIsConditionalJump()); - if (block->KindIs(BBJ_COND) && (newNext != nullptr) && block->TrueTargetIs(newNext)) + if (test->OperGet() == GT_JTRUE) { - // Reverse the jump condition - GenTree* test = block->lastNode(); - noway_assert(test->OperIsConditionalJump()); - - if (test->OperGet() == GT_JTRUE) - { - GenTree* cond = gtReverseCond(test->AsOp()->gtOp1); - assert(cond == test->AsOp()->gtOp1); // Ensure `gtReverseCond` did not create a new node. - test->AsOp()->gtOp1 = cond; - } - else - { - gtReverseCond(test); - } - - // Redirect the Conditional JUMP to go to `oldNext` - block->SetTrueTarget(oldNext); - block->SetFalseTarget(newNext); + GenTree* cond = gtReverseCond(test->AsOp()->gtOp1); + assert(cond == test->AsOp()->gtOp1); // Ensure `gtReverseCond` did not create a new node. + test->AsOp()->gtOp1 = cond; } else { - // Insert an unconditional jump to `oldNext` just after `block`. - fgConnectFallThrough(block, oldNext); + gtReverseCond(test); } + // Redirect the Conditional JUMP to go to `oldNext` + block->SetTrueTarget(block->GetFalseTarget()); + block->SetFalseTarget(newNext); changed = true; } else if (block->KindIs(BBJ_ALWAYS) && block->TargetIs(newNext)) @@ -3075,7 +3046,7 @@ bool Compiler::optCreatePreheader(FlowGraphNaturalLoop* loop) BasicBlock* preheaderCandidate = loop->EntryEdges()[0]->getSourceBlock(); unsigned candidateEHRegion = preheaderCandidate->hasTryIndex() ? preheaderCandidate->getTryIndex() : EHblkDsc::NO_ENCLOSING_INDEX; - if (preheaderCandidate->KindIs(BBJ_ALWAYS) && (preheaderCandidate->GetUniqueSucc() == loop->GetHeader()) && + if (preheaderCandidate->KindIs(BBJ_ALWAYS) && preheaderCandidate->TargetIs(loop->GetHeader()) && (candidateEHRegion == preheaderEHRegion)) { JITDUMP("Natural loop " FMT_LP " already has preheader " FMT_BB "\n", loop->GetIndex(), @@ -3113,47 +3084,6 @@ bool Compiler::optCreatePreheader(FlowGraphNaturalLoop* loop) header->bbNum, enterBlock->bbNum, preheader->bbNum); fgReplaceJumpTarget(enterBlock, preheader, header); - - // Fix up fall through - if (enterBlock->KindIs(BBJ_COND) && enterBlock->NextIs(header)) - { - FlowEdge* edge = fgRemoveRefPred(header, enterBlock); - BasicBlock* newAlways = fgNewBBafter(BBJ_ALWAYS, enterBlock, true, preheader); - fgAddRefPred(preheader, newAlways, edge); - fgAddRefPred(newAlways, enterBlock, edge); - - JITDUMP("Created new BBJ_ALWAYS " FMT_BB " to redirect " FMT_BB " to preheader\n", newAlways->bbNum, - enterBlock->bbNum); - enterBlock->SetFalseTarget(newAlways); - } - } - - // Fix up potential fallthrough into the preheader. - BasicBlock* fallthroughSource = preheader->Prev(); - if ((fallthroughSource != nullptr) && fallthroughSource->KindIs(BBJ_COND)) - { - if (!loop->ContainsBlock(fallthroughSource)) - { - // Either unreachable or an enter edge. The new fallthrough into - // the preheader is what we want. We still need to update refs - // which fgReplaceJumpTarget doesn't do for fallthrough. - FlowEdge* old = fgRemoveRefPred(header, fallthroughSource); - fgAddRefPred(preheader, fallthroughSource, old); - } - else - { - // For a backedge we need to make sure we're still going to the head, - // and not falling into the preheader. - FlowEdge* edge = fgRemoveRefPred(header, fallthroughSource); - BasicBlock* newAlways = fgNewBBafter(BBJ_ALWAYS, fallthroughSource, true, header); - fgAddRefPred(header, newAlways, edge); - fgAddRefPred(newAlways, fallthroughSource, edge); - - JITDUMP("Created new BBJ_ALWAYS " FMT_BB " to redirect " FMT_BB " over preheader\n", newAlways->bbNum, - fallthroughSource->bbNum); - } - - fallthroughSource->SetFalseTarget(fallthroughSource->Next()); } optSetPreheaderWeight(loop, preheader); diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 0201a62ddf7b7f..4241a30ab579b6 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1727,12 +1727,11 @@ bool Compiler::optJumpThreadCore(JumpThreadInfo& jti) { Statement* const lastStmt = jti.m_block->lastStmt(); fgRemoveStmt(jti.m_block, lastStmt); - JITDUMP(" repurposing " FMT_BB " to always fall through to " FMT_BB "\n", jti.m_block->bbNum, + JITDUMP(" repurposing " FMT_BB " to always jump to " FMT_BB "\n", jti.m_block->bbNum, jti.m_falseTarget->bbNum); fgRemoveRefPred(jti.m_trueTarget, jti.m_block); jti.m_block->SetKindAndTarget(BBJ_ALWAYS, jti.m_falseTarget); jti.m_block->SetFlags(BBF_NONE_QUIRK); - assert(jti.m_block->JumpsToNext()); } // Now reroute the flow from the predecessors. From 0d8ae08ff2b8e73ba01cd619980437e787d0895b Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 24 Jan 2024 20:04:36 -0500 Subject: [PATCH 06/12] Quirk fgReplaceJumpTarget --- src/coreclr/jit/fgbasic.cpp | 14 ++++++++++---- src/coreclr/jit/fgopt.cpp | 4 ++-- src/coreclr/jit/flowgraph.cpp | 3 +-- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 25f56fdc88ce6b..65791968783d0c 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -694,14 +694,15 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* newTarget, Bas case BBJ_COND: { - FlowEdge* oldEdge = fgGetPredForBlock(oldTarget, block); + FlowEdge* oldEdge = fgRemoveRefPred(oldTarget, block); assert(oldEdge != nullptr); if (block->TrueTargetIs(oldTarget)) { if (block->FalseTargetIs(oldTarget)) { - assert(oldEdge->getDupCount() == 2); + // Above call to fgRemoveRefPred decremented count from 2 + assert(oldEdge->getDupCount() == 1); fgRemoveConditionalJump(block); assert(block->KindIs(BBJ_ALWAYS)); block->SetTarget(newTarget); @@ -710,15 +711,20 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* newTarget, Bas { block->SetTrueTarget(newTarget); } + + // TODO-NoFallThrough: Proliferate weight from oldEdge + // (we avoid doing so when updating the true target to reduce diffs for now, as a quirk) + fgAddRefPred(newTarget, block); } else { assert(block->FalseTargetIs(oldTarget)); block->SetFalseTarget(newTarget); + fgAddRefPred(newTarget, block, oldEdge); } - assert(oldEdge->getDupCount() == 1); - fgAddRefPred(newTarget, block, fgRemoveRefPred(oldTarget, block)); + // block should not have any edges to oldTarget by now + assert(oldEdge->getDupCount() == 0); break; } diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 204e58f4435931..3c52e0ea901563 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4951,8 +4951,8 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh { fgRemoveConditionalJump(block); assert(block->KindIs(BBJ_ALWAYS)); - change = true; - modified = true; + change = true; + modified = true; bFalseDest = nullptr; } } diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 56ad882b820f69..b778477489b5ed 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4375,8 +4375,7 @@ FlowGraphNaturalLoops* FlowGraphNaturalLoops::Find(const FlowGraphDfsTree* dfsTr BasicBlock* predBlock = predEdge->getSourceBlock(); if (dfsTree->Contains(predBlock) && !dfsTree->IsAncestor(header, predBlock)) { - JITDUMP(FMT_BB " -> " FMT_BB " is an entry edge\n", predBlock->bbNum, - loop->m_header->bbNum); + JITDUMP(FMT_BB " -> " FMT_BB " is an entry edge\n", predBlock->bbNum, loop->m_header->bbNum); loop->m_entryEdges.push_back(predEdge); } } From 6eac1b3d23cd7bf9aaf4fbfebdaa32438c5c88b8 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 24 Jan 2024 21:26:02 -0500 Subject: [PATCH 07/12] Clean up --- src/coreclr/jit/block.h | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index e42bab3b1b3576..b94e482d5d91cc 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -675,12 +675,7 @@ struct BasicBlock : private LIR::Range BasicBlock* GetFalseTarget() const { assert(KindIs(BBJ_COND)); - - // So long as bbFalseTarget tracks bbNext in SetNext(), it is possible for bbFalseTarget to be null - // if this block is unlinked from the block list. - // So check bbNext before triggering the assert if bbFalseTarget is null. - // TODO-NoFallThrough: Remove IsLast() check once bbFalseTarget isn't hard-coded to bbNext - assert((bbFalseTarget != nullptr) || IsLast()); + assert(bbFalseTarget != nullptr); return bbFalseTarget; } From dbd739ae3a321210ad490bd307ca9fb705b01b2e Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 25 Jan 2024 11:15:01 -0500 Subject: [PATCH 08/12] Fix assert --- src/coreclr/jit/fgopt.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 3c52e0ea901563..c8c1d0b9e5e078 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3699,7 +3699,10 @@ bool Compiler::fgReorderBlocks(bool useProfile) } else if (bPrev->KindIs(BBJ_COND)) { - assert(bPrev->FalseTargetIs(block)); + // fgReorderBlocks is called in more than one optimization phase, + // but only does any reordering in optOptimizeLayout. + // At that point, we expect implicit fallthrough to be restored for BBJ_COND blocks. + assert(bPrev->FalseTargetIs(block) || !reorderBlock); bDest = bPrev->GetTrueTarget(); forwardBranch = fgIsForwardBranch(bPrev, bDest); backwardBranch = !forwardBranch; From 5a46d1f767cfbbb204e43d91cb3ad85c1774eb5c Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 25 Jan 2024 11:55:45 -0500 Subject: [PATCH 09/12] Avoid divide by zero warning --- src/coreclr/jit/fgprofilesynthesis.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgprofilesynthesis.cpp b/src/coreclr/jit/fgprofilesynthesis.cpp index 923d3276664988..b7af7de958555f 100644 --- a/src/coreclr/jit/fgprofilesynthesis.cpp +++ b/src/coreclr/jit/fgprofilesynthesis.cpp @@ -337,7 +337,9 @@ void ProfileSynthesis::AssignLikelihoodSwitch(BasicBlock* block) // const unsigned n = block->NumSucc(); assert(n != 0); - const weight_t p = 1 / (weight_t)n; + + // Check for divide by zero to avoid compiler warnings for Release builds (above assert is removed) + const weight_t p = (n != 0) ? (1 / (weight_t)n) : 0; // Each unique edge gets some multiple of that basic probability // From 046cb67b6c5f0e5e61e3d86742f4b24b55e5f1b5 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 25 Jan 2024 12:03:25 -0500 Subject: [PATCH 10/12] Fix loop condition --- src/coreclr/jit/optimizer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 976e17a6859cd7..c4b3f1fe8d3b0c 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2846,7 +2846,7 @@ BasicBlock* Compiler::optFindLoopCompactionInsertionPoint(FlowGraphNaturalLoop* // out of the loop, and if possible find a spot that won't break up fall-through. BasicBlock* bottom = loop->GetLexicallyBottomMostBlock(); BasicBlock* insertionPoint = bottom; - while (insertionPoint->bbFallsThrough()) + while (insertionPoint->bbFallsThrough() && !insertionPoint->IsLast()) { // Keep looking for a better insertion point if we can. BasicBlock* newInsertionPoint = optTryAdvanceLoopCompactionInsertionPoint(loop, insertionPoint, top, bottom); From d403274e6def5a946081c3f352d05501ceeddc7c Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 25 Jan 2024 12:46:35 -0500 Subject: [PATCH 11/12] Remove old case in fgConnectFallThrough --- src/coreclr/jit/fgbasic.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 65791968783d0c..1ee0353006f284 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -5575,10 +5575,6 @@ 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; } From 53df4c900b17de32b0ae085789b6a6238783ff32 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 25 Jan 2024 13:26:16 -0500 Subject: [PATCH 12/12] Fix potential nullptr deref --- src/coreclr/jit/fgbasic.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 1ee0353006f284..cd7498a3b86f0f 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -695,20 +695,20 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* newTarget, Bas case BBJ_COND: { FlowEdge* oldEdge = fgRemoveRefPred(oldTarget, block); - assert(oldEdge != nullptr); - if (block->TrueTargetIs(oldTarget)) { if (block->FalseTargetIs(oldTarget)) { - // Above call to fgRemoveRefPred decremented count from 2 - assert(oldEdge->getDupCount() == 1); + // fgRemoveRefPred returns nullptr for BBJ_COND blocks with two flow edges to target + assert(oldEdge == nullptr); fgRemoveConditionalJump(block); assert(block->KindIs(BBJ_ALWAYS)); block->SetTarget(newTarget); } else { + // fgRemoveRefPred should have removed the flow edge + assert(oldEdge != nullptr); block->SetTrueTarget(newTarget); } @@ -718,13 +718,12 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* newTarget, Bas } else { + // fgRemoveRefPred should have removed the flow edge + assert(oldEdge != nullptr); assert(block->FalseTargetIs(oldTarget)); block->SetFalseTarget(newTarget); fgAddRefPred(newTarget, block, oldEdge); } - - // block should not have any edges to oldTarget by now - assert(oldEdge->getDupCount() == 0); break; }