From 5598dac557c35d264c527e22239f1b33e0489376 Mon Sep 17 00:00:00 2001 From: Aman Khalid Date: Wed, 17 Jan 2024 15:02:21 -0500 Subject: [PATCH] JIT: Allow BBJ_COND false target to diverge from bbNext in layout optimization phase (#96609) Next step for #93020. Working backwards through the JIT flowgraph phases, this change allows bbFalseTarget to diverge from bbNext in Compiler::optOptimizeLayout and onwards. --- src/coreclr/jit/block.cpp | 14 +- src/coreclr/jit/block.h | 10 +- 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 | 26 +- src/coreclr/jit/codegenriscv64.cpp | 7 + src/coreclr/jit/codegenxarch.cpp | 7 + src/coreclr/jit/compiler.h | 4 +- src/coreclr/jit/fgbasic.cpp | 96 ++++++-- src/coreclr/jit/fgdiagnostic.cpp | 9 +- src/coreclr/jit/fgopt.cpp | 326 +++++++++++-------------- src/coreclr/jit/fgprofilesynthesis.cpp | 5 +- src/coreclr/jit/jiteh.cpp | 7 - src/coreclr/jit/lower.cpp | 2 +- src/coreclr/jit/optimizer.cpp | 56 ++--- src/coreclr/jit/stacklevelsetter.cpp | 23 ++ src/coreclr/jit/switchrecognition.cpp | 30 ++- 19 files changed, 364 insertions(+), 286 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 446733c80853c..24d37e691c9ff 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 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); } //------------------------------------------------------------------------ @@ -1170,7 +1172,7 @@ unsigned BasicBlock::NumSucc() const return 1; case BBJ_COND: - if (bbTarget == bbNext) + if (bbTrueTarget == bbFalseTarget) { return 1; } @@ -1295,7 +1297,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 40405678fae98..2eef62974eed3 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 { @@ -669,7 +669,6 @@ struct BasicBlock : private LIR::Range { assert(KindIs(BBJ_COND)); assert(bbTrueTarget != nullptr); - assert(target != nullptr); return (bbTrueTarget == target); } @@ -696,15 +695,16 @@ struct BasicBlock : private LIR::Range { assert(KindIs(BBJ_COND)); assert(bbFalseTarget != nullptr); - assert(target != nullptr); return (bbFalseTarget == target); } void SetCond(BasicBlock* trueTarget, BasicBlock* falseTarget) { + // Switch lowering may temporarily set a block to a BBJ_COND + // with a null false target if it is the last block in the list. + // This invalid state is eventually fixed, so allow it in the below assert. + assert((falseTarget != nullptr) || (falseTarget == bbNext)); 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/codegenarm.cpp b/src/coreclr/jit/codegenarm.cpp index 28908f4392d79..4a8c08a89858e 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 d9681d12012eb..489aa6a744942 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 09a28cc373bf4..f0471d242d812 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 83712ec0cd669..81c84a10f45a2 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -2621,9 +2621,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 1311ccc082e80..0a15bdf1dc7c8 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); + } } //--------------------------------------------------------------------- @@ -4874,16 +4881,31 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode) case GT_JCC: { +#if !FEATURE_FIXED_OUT_ARGS BasicBlock* tgtBlock = compiler->compCurBB->KindIs(BBJ_COND) ? compiler->compCurBB->GetTrueTarget() : compiler->compCurBB->GetTarget(); -#if !FEATURE_FIXED_OUT_ARGS assert((tgtBlock->bbTgtStkDepth * sizeof(int) == genStackLevel) || isFramePointerUsed()); #endif // !FEATURE_FIXED_OUT_ARGS GenTreeCC* jcc = treeNode->AsCC(); 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)) + { + emit->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), (int)1 /* cc */); + + // 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); + } + } + else + { + emit->emitIns_J(ins, compiler->compCurBB->GetTarget(), (int)1 /* cc */); + } } break; diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index d240baee699da..f3bc1b10c5bdb 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3920,6 +3920,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 1aac2b8890273..1d2e0d71768b3 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -1453,6 +1453,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); + } } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index c08f99c396c0b..b0c2f05b6c03f 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5956,7 +5956,7 @@ class Compiler void fgUpdateLoopsAfterCompacting(BasicBlock* block, BasicBlock* bNext); - BasicBlock* fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst); + BasicBlock* fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst, bool noFallThroughQuirk = false); bool fgRenumberBlocks(); @@ -6003,8 +6003,6 @@ class Compiler bool fgOptimizeSwitchBranches(BasicBlock* block); - bool fgOptimizeBranchToNext(BasicBlock* block, BasicBlock* bNext, BasicBlock* bPrev); - bool fgOptimizeSwitchJumps(); #ifdef DEBUG void fgPrintEdgeWeights(); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 1d3a3915aea30..d88ed59a8f196 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -693,15 +693,35 @@ 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); + fgRemoveRefPred(oldTarget, block); + fgAddRefPred(newTarget, block); break; + } case BBJ_SWITCH: { @@ -5063,13 +5083,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)) @@ -5279,6 +5303,12 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable) /* At this point the bbPreds and bbRefs had better be zero */ noway_assert((block->bbRefs == 0) && (block->bbPreds == nullptr)); + + if (bPrev->KindIs(BBJ_COND) && bPrev->FalseTargetIs(block)) + { + assert(bNext != nullptr); + bPrev->SetFalseTarget(bNext); + } } else // block is empty { @@ -5388,24 +5418,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: @@ -5440,7 +5461,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())) @@ -5508,7 +5531,7 @@ void Compiler::fgPrepareCallFinallyRetForRemoval(BasicBlock* block) // Newly inserted block after bSrc that jumps to bDst, // or nullptr if bSrc already falls through to bDst // -BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst) +BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst, bool noFallThroughQuirk /* = false */) { assert(bSrc != nullptr); assert(fgPredsComputed); @@ -5516,8 +5539,18 @@ 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)) { + assert(fgGetPredForBlock(bDst, bSrc) != nullptr); + + // Allow the caller to decide whether to use the old logic of maintaining implicit fallthrough behavior, + // or to allow BBJ_COND blocks' false targets to diverge from bbNext. + // TODO-NoFallThrough: Remove this quirk once callers' dependencies on implicit fallthrough are gone. + if (noFallThroughQuirk) + { + return jmpBlk; + } + // Add a new block after bSrc which jumps to 'bDst' jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst); bSrc->SetFalseTarget(jmpBlk); @@ -5691,8 +5724,14 @@ bool Compiler::fgRenumberBlocks() */ bool Compiler::fgIsForwardBranch(BasicBlock* bJump, BasicBlock* bDest, BasicBlock* bSrc /* = NULL */) { - assert((bJump->KindIs(BBJ_ALWAYS, BBJ_CALLFINALLYRET) && bJump->TargetIs(bDest)) || - (bJump->KindIs(BBJ_COND) && bJump->TrueTargetIs(bDest))); + if (bJump->KindIs(BBJ_COND)) + { + assert(bJump->TrueTargetIs(bDest) || bJump->FalseTargetIs(bDest)); + } + else + { + assert(bJump->KindIs(BBJ_ALWAYS, BBJ_CALLFINALLYRET) && bJump->TargetIs(bDest)); + } bool result = false; BasicBlock* bTemp = (bSrc == nullptr) ? bJump : bSrc; @@ -6627,14 +6666,17 @@ BasicBlock* Compiler::fgFindInsertPoint(unsigned regionIndex, // Look for an insert location: // 1. We want blocks that don't end with a fall through, // 2. Also, when blk equals nearBlk we may want to insert here. + // Quirk: Manually check for BBJ_COND fallthrough behavior + const bool blkFallsThrough = + blk->bbFallsThrough() && (!blk->KindIs(BBJ_COND) || blk->NextIs(blk->GetFalseTarget())); const bool blkJumpsToNext = blk->KindIs(BBJ_ALWAYS) && blk->HasFlag(BBF_NONE_QUIRK) && blk->JumpsToNext(); - if ((!blk->bbFallsThrough() && !blkJumpsToNext) || (blk == nearBlk)) + if ((!blkFallsThrough && !blkJumpsToNext) || (blk == nearBlk)) { bool updateBestBlk = true; // We will probably update the bestBlk // If blk falls through then we must decide whether to use the nearBlk // hint - if (blk->bbFallsThrough() || blkJumpsToNext) + if (blkFallsThrough || blkJumpsToNext) { noway_assert(blk == nearBlk); if (jumpBlk != nullptr) diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 95e6b6a858f6a..2fbf905d131ac 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -136,7 +136,7 @@ void Compiler::fgDebugCheckUpdate() // A conditional branch should never jump to the next block as it can be folded into a BBJ_ALWAYS. if (block->KindIs(BBJ_COND) && block->TrueTargetIs(block->GetFalseTarget())) { - noway_assert(!"Unnecessary jump to the next block!"); + noway_assert(!"BBJ_COND true/false targets are the same!"); } // For a BBJ_CALLFINALLY block we make sure that we are followed by a BBJ_CALLFINALLYRET block @@ -2977,13 +2977,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 b48f1450a1a85..996ed5dd16f0e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1516,109 +1516,6 @@ void Compiler::fgUnreachableBlock(BasicBlock* block) fgRemoveBlockAsPred(block); } -//------------------------------------------------------------- -// fgRemoveConditionalJump: Remove or morph a jump when we jump to the same -// block when both the condition is true or false. Remove the branch condition, -// but leave any required side effects. -// -// Arguments: -// block - block with conditional branch -// -void Compiler::fgRemoveConditionalJump(BasicBlock* block) -{ - noway_assert(block->KindIs(BBJ_COND) && block->TrueTargetIs(block->GetFalseTarget())); - assert(compRationalIRForm == block->IsLIR()); - - FlowEdge* flow = fgGetPredForBlock(block->GetFalseTarget(), block); - noway_assert(flow->getDupCount() == 2); - - // Change the BBJ_COND to BBJ_ALWAYS, and adjust the refCount and dupCount. - --block->GetFalseTarget()->bbRefs; - block->SetKind(BBJ_ALWAYS); - block->SetFlags(BBF_NONE_QUIRK); - flow->decrementDupCount(); - -#ifdef DEBUG - if (verbose) - { - printf("Block " FMT_BB " becoming a BBJ_ALWAYS to " FMT_BB " (jump target is the same whether the condition" - " is true or false)\n", - block->bbNum, block->GetTarget()->bbNum); - } -#endif - - // Remove the block jump condition - - if (block->IsLIR()) - { - LIR::Range& blockRange = LIR::AsRange(block); - - GenTree* test = blockRange.LastNode(); - assert(test->OperIsConditionalJump()); - - bool isClosed; - unsigned sideEffects; - LIR::ReadOnlyRange testRange = blockRange.GetTreeRange(test, &isClosed, &sideEffects); - - // TODO-LIR: this should really be checking GTF_ALL_EFFECT, but that produces unacceptable - // diffs compared to the existing backend. - if (isClosed && ((sideEffects & GTF_SIDE_EFFECT) == 0)) - { - // If the jump and its operands form a contiguous, side-effect-free range, - // remove them. - blockRange.Delete(this, block, std::move(testRange)); - } - else - { - // Otherwise, just remove the jump node itself. - blockRange.Remove(test, true); - } - } - else - { - Statement* test = block->lastStmt(); - GenTree* tree = test->GetRootNode(); - - noway_assert(tree->gtOper == GT_JTRUE); - - GenTree* sideEffList = nullptr; - - if (tree->gtFlags & GTF_SIDE_EFFECT) - { - gtExtractSideEffList(tree, &sideEffList); - - if (sideEffList) - { - noway_assert(sideEffList->gtFlags & GTF_SIDE_EFFECT); -#ifdef DEBUG - if (verbose) - { - printf("Extracted side effects list from condition...\n"); - gtDispTree(sideEffList); - printf("\n"); - } -#endif - } - } - - // Delete the cond test or replace it with the side effect tree - if (sideEffList == nullptr) - { - fgRemoveStmt(block, test); - } - else - { - test->SetRootNode(sideEffList); - - if (fgNodeThreading != NodeThreading::None) - { - gtSetStmtInfo(test); - fgSetStmtSeq(test); - } - } - } -} - //------------------------------------------------------------- // fgOptimizeBranchToEmptyUnconditional: // Optimize a jump to an empty block which ends in an unconditional branch. @@ -2684,27 +2581,25 @@ bool Compiler::fgOptimizeUncondBranchToSimpleCond(BasicBlock* block, BasicBlock* } //------------------------------------------------------------- -// fgOptimizeBranchToNext: -// Optimize a block which has a branch to the following block +// fgRemoveConditionalJump: Remove or morph a jump when we jump to the same +// block when both the condition is true or false. Remove the branch condition, +// but leave any required side effects. // // Arguments: -// block - block with a BBJ_COND jump kind -// bNext - target that block jumps to regardless of its condition -// bPrev - block which is prior to the first block -// -// Returns: true if changes were made +// block - block with conditional branch // -bool Compiler::fgOptimizeBranchToNext(BasicBlock* block, BasicBlock* bNext, BasicBlock* bPrev) +void Compiler::fgRemoveConditionalJump(BasicBlock* block) { assert(block->KindIs(BBJ_COND)); - assert(block->TrueTargetIs(bNext)); - assert(block->FalseTargetIs(bNext)); - assert(block->PrevIs(bPrev)); + assert(block->FalseTargetIs(block->GetTrueTarget())); + BasicBlock* target = block->GetTrueTarget(); #ifdef DEBUG if (verbose) { - printf("\nRemoving conditional jump to next block (" FMT_BB " -> " FMT_BB ")\n", block->bbNum, bNext->bbNum); + printf("Block " FMT_BB " becoming a BBJ_ALWAYS to " FMT_BB " (jump target is the same whether the condition" + " is true or false)\n", + block->bbNum, target->bbNum); } #endif // DEBUG @@ -2803,17 +2698,20 @@ bool Compiler::fgOptimizeBranchToNext(BasicBlock* block, BasicBlock* bNext, Basi } } - /* Conditional is gone - always jump to the next block */ + /* Conditional is gone - always jump to target */ block->SetKind(BBJ_ALWAYS); + if (block->JumpsToNext()) + { + block->SetFlags(BBF_NONE_QUIRK); + } + /* Update bbRefs and bbNum - Conditional predecessors to the same * block are counted twice so we have to remove one of them */ - noway_assert(bNext->countOfInEdges() > 1); - fgRemoveRefPred(bNext, block); - - return true; + noway_assert(target->countOfInEdges() > 1); + fgRemoveRefPred(target, block); } //------------------------------------------------------------- @@ -3736,11 +3634,13 @@ bool Compiler::fgReorderBlocks(bool useProfile) continue; } - bool reorderBlock = useProfile; - const bool isRare = block->isRunRarely(); - BasicBlock* bDest = nullptr; - bool forwardBranch = false; - bool backwardBranch = false; + bool reorderBlock = useProfile; + const bool isRare = block->isRunRarely(); + BasicBlock* bDest = nullptr; + BasicBlock* bFalseDest = nullptr; + bool forwardBranch = false; + bool backwardBranch = false; + bool forwardFalseBranch = false; // Setup bDest if (bPrev->KindIs(BBJ_ALWAYS, BBJ_CALLFINALLYRET)) @@ -3751,9 +3651,13 @@ bool Compiler::fgReorderBlocks(bool useProfile) } else if (bPrev->KindIs(BBJ_COND)) { - bDest = bPrev->GetTrueTarget(); - forwardBranch = fgIsForwardBranch(bPrev, bDest); - backwardBranch = !forwardBranch; + assert(bPrev->FalseTargetIs(block) || useProfile); + bDest = bPrev->GetTrueTarget(); + bFalseDest = bPrev->GetFalseTarget(); + // TODO: Cheaper to call fgRenumberBlocks first and compare bbNums? + forwardBranch = fgIsForwardBranch(bPrev, bDest); + backwardBranch = !forwardBranch; + forwardFalseBranch = fgIsForwardBranch(bPrev, bFalseDest); } // We will look for bPrev as a non rarely run block followed by block as a rarely run block @@ -3886,43 +3790,55 @@ bool Compiler::fgReorderBlocks(bool useProfile) // We will check that the min weight of the bPrev to bDest edge // is more than twice the max weight of the bPrev to block edge. // - // bPrev --> [BB04, weight 31] + // bPrev --------> [BB04, weight 31] // | \. - // edgeToBlock -------------> O \. + // falseEdge ---------------> O \. // [min=8,max=10] V \. - // block --> [BB05, weight 10] \. + // bFalseDest ---> [BB05, weight 10] \. // \. - // edgeToDest ----------------------------> O + // trueEdge ------------------------------> O // [min=21,max=23] | // V // bDest ---------------> [BB08, weight 21] // - FlowEdge* edgeToDest = fgGetPredForBlock(bDest, bPrev); - FlowEdge* edgeToBlock = fgGetPredForBlock(block, bPrev); - noway_assert(edgeToDest != nullptr); - noway_assert(edgeToBlock != nullptr); + FlowEdge* trueEdge = fgGetPredForBlock(bDest, bPrev); + FlowEdge* falseEdge = fgGetPredForBlock(bFalseDest, bPrev); + noway_assert(trueEdge != nullptr); + noway_assert(falseEdge != nullptr); // // Calculate the taken ratio - // A takenRation of 0.10 means taken 10% of the time, not taken 90% of the time - // A takenRation of 0.50 means taken 50% of the time, not taken 50% of the time - // A takenRation of 0.90 means taken 90% of the time, not taken 10% of the time + // A takenRatio of 0.10 means taken 10% of the time, not taken 90% of the time + // A takenRatio of 0.50 means taken 50% of the time, not taken 50% of the time + // A takenRatio of 0.90 means taken 90% of the time, not taken 10% of the time // double takenCount = - ((double)edgeToDest->edgeWeightMin() + (double)edgeToDest->edgeWeightMax()) / 2.0; + ((double)trueEdge->edgeWeightMin() + (double)trueEdge->edgeWeightMax()) / 2.0; double notTakenCount = - ((double)edgeToBlock->edgeWeightMin() + (double)edgeToBlock->edgeWeightMax()) / 2.0; + ((double)falseEdge->edgeWeightMin() + (double)falseEdge->edgeWeightMax()) / 2.0; double totalCount = takenCount + notTakenCount; // If the takenRatio (takenCount / totalCount) is greater or equal to 51% then we will reverse // the branch if (takenCount < (0.51 * totalCount)) { - reorderBlock = false; + // We take bFalseDest more often. + // If bFalseDest is a backward branch, we should reverse the condition. + // Else if both branches are forward, not much use in reversing the condition. + reorderBlock = !forwardFalseBranch; + } + else if (bFalseDest == block) + { + // We take bDest more often, and we can fall into bFalseDest, + // so it makes sense to reverse the condition. + profHotWeight = (falseEdge->edgeWeightMin() + falseEdge->edgeWeightMax()) / 2 - 1; } else { - // set profHotWeight - profHotWeight = (edgeToBlock->edgeWeightMin() + edgeToBlock->edgeWeightMax()) / 2 - 1; + // We take bDest more often than bFalseDest, but bFalseDest isn't the next block, + // so reversing the branch doesn't make sense since bDest is already a forward branch. + assert(takenCount >= (0.51 * totalCount)); + assert(bFalseDest != block); + reorderBlock = false; } } else @@ -3956,8 +3872,16 @@ bool Compiler::fgReorderBlocks(bool useProfile) profHotWeight = (weightDest < weightPrev) ? weightDest : weightPrev; // if the weight of block is greater (or equal) to profHotWeight then we don't reverse the cond - if (block->bbWeight >= profHotWeight) + if (bDest->bbWeight >= profHotWeight) + { + // bFalseDest has a greater weight, so if it is a backward branch, + // we should reverse the branch. + reorderBlock = !forwardFalseBranch; + } + else if (bFalseDest != block) { + // bFalseDest is taken less often, but it isn't the next block, + // so it doesn't make sense to reverse the branch. reorderBlock = false; } } @@ -4019,7 +3943,9 @@ bool Compiler::fgReorderBlocks(bool useProfile) } const bool bTmpJumpsToNext = bTmp->KindIs(BBJ_ALWAYS, BBJ_CALLFINALLYRET) && bTmp->JumpsToNext(); - if ((!bTmp->bbFallsThrough() && !bTmpJumpsToNext) || (bTmp->bbWeight == BB_ZERO_WEIGHT)) + const bool bTmpFallsThrough = + bTmp->bbFallsThrough() && (!bTmp->KindIs(BBJ_COND) || bTmp->NextIs(bTmp->GetFalseTarget())); + if ((!bTmpJumpsToNext && !bTmpFallsThrough) || (bTmp->bbWeight == BB_ZERO_WEIGHT)) { lastNonFallThroughBlock = bTmp; } @@ -4211,7 +4137,7 @@ bool Compiler::fgReorderBlocks(bool useProfile) } // Set connected_bDest to true if moving blocks [bStart .. bEnd] - // connects with the jump dest of bPrev (i.e bDest) and + // connects with the jump dest of bPrev (i.e bDest) and // thus allows bPrev fall through instead of jump. if (bNext == bDest) { @@ -4290,7 +4216,12 @@ bool Compiler::fgReorderBlocks(bool useProfile) { // Treat jumps to next block as fall-through } - else if (bEnd2->bbFallsThrough() == false) + else if (bEnd2->KindIs(BBJ_COND) && !bEnd2->FalseTargetIs(bNext)) + { + // Block does not fall through into false target + break; + } + else if (!bEnd2->bbFallsThrough()) { break; } @@ -4378,7 +4309,7 @@ bool Compiler::fgReorderBlocks(bool useProfile) } else { - if (bPrev->bbFallsThrough()) + if (bPrev->KindIs(BBJ_COND) && bPrev->NextIs(bPrev->GetFalseTarget())) { printf("since it falls into a rarely run block\n"); } @@ -4474,6 +4405,8 @@ bool Compiler::fgReorderBlocks(bool useProfile) // Find new location for the unlinked block(s) // Set insertAfterBlk to the block which will precede the insertion point + EHblkDsc* ehDsc; + if (!bStart->hasTryIndex() && isRare) { // We'll just insert the blocks at the end of the method. If the method @@ -4488,7 +4421,7 @@ bool Compiler::fgReorderBlocks(bool useProfile) { BasicBlock* startBlk; BasicBlock* lastBlk; - EHblkDsc* ehDsc = ehInitTryBlockRange(bStart, &startBlk, &lastBlk); + ehDsc = ehInitTryBlockRange(bStart, &startBlk, &lastBlk); BasicBlock* endBlk; @@ -4730,6 +4663,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) @@ -4739,18 +4677,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); } } @@ -4789,32 +4719,35 @@ bool Compiler::fgReorderBlocks(bool useProfile) /* We have decided to insert the block(s) after 'insertAfterBlk' */ fgMoveBlocksAfter(bStart, bEnd, insertAfterBlk); + // useProfile should be true only when finalizing the block layout in Compiler::optOptimizeLayout. + // In this final pass, allow BBJ_COND blocks' false targets to diverge from bbNext. + // TODO-NoFallThrough: Always allow the false targets to diverge. if (bDest) { /* We may need to insert an unconditional branch after bPrev to bDest */ - fgConnectFallThrough(bPrev, bDest); + fgConnectFallThrough(bPrev, bDest, /* noFallThroughQuirk */ useProfile); } else { /* If bPrev falls through, we must insert a jump to block */ - fgConnectFallThrough(bPrev, block); + fgConnectFallThrough(bPrev, block, /* noFallThroughQuirk */ useProfile); } BasicBlock* bSkip = bEnd->Next(); /* If bEnd falls through, we must insert a jump to bNext */ - fgConnectFallThrough(bEnd, bNext); + fgConnectFallThrough(bEnd, bNext, /* noFallThroughQuirk */ useProfile); if (bStart2 == nullptr) { /* If insertAfterBlk falls through, we are forced to */ /* add a jump around the block(s) we just inserted */ - fgConnectFallThrough(insertAfterBlk, bSkip); + fgConnectFallThrough(insertAfterBlk, bSkip, /* noFallThroughQuirk */ useProfile); } else { /* We may need to insert an unconditional branch after bPrev2 to bStart */ - fgConnectFallThrough(bPrev2, bStart); + fgConnectFallThrough(bPrev2, bStart, /* noFallThroughQuirk */ useProfile); } #if DEBUG @@ -5001,25 +4934,41 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) } else if (block->KindIs(BBJ_COND)) { - bDest = block->GetTrueTarget(); - if (bDest == bNext) + bDest = block->GetTrueTarget(); + BasicBlock* bFalseDest = block->GetFalseTarget(); + assert(bFalseDest != nullptr); + + if (bFalseDest->KindIs(BBJ_ALWAYS) && bFalseDest->TargetIs(bDest) && bFalseDest->isEmpty()) { - // 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; - } + // Optimize bFalseDest -> BBJ_ALWAYS -> bDest + block->SetFalseTarget(bDest); + fgRemoveRefPred(bFalseDest, block); + fgAddRefPred(bDest, block); + } + else if (bDest->KindIs(BBJ_ALWAYS) && bDest->TargetIs(bFalseDest) && bDest->isEmpty()) + { + // Optimize bDest -> BBJ_ALWAYS -> bFalseDest + block->SetTrueTarget(bFalseDest); + fgRemoveRefPred(bDest, block); + fgAddRefPred(bFalseDest, block); + bDest = bFalseDest; + } + + if (block->FalseTargetIs(bDest)) + { + fgRemoveConditionalJump(block); + change = true; + modified = true; + assert(block->KindIs(BBJ_ALWAYS)); + assert(block->TargetIs(bDest)); } } 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); @@ -5039,18 +4988,18 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) // (b) block jump target is elsewhere but join free, and // 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 - !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 + if (block->KindIs(BBJ_COND) && // block is a BBJ_COND block + (bNext != nullptr) && // block isn't the last block + block->FalseTargetIs(bNext) && // block falls into its false target + (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); + assert(!block->IsLast()); // case (a) // @@ -5106,6 +5055,12 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) } } + // TODO-NoFallThrough: Remove this requirement + if (bDest->KindIs(BBJ_COND) && !bDest->NextIs(bDest->GetFalseTarget())) + { + optimizeJump = false; + } + if (optimizeJump && isJumpToJoinFree) { // In the join free case, we also need to move bDest right after bNext @@ -5186,6 +5141,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase) } // Optimize the Conditional JUMP to go to the new target + assert(block->FalseTargetIs(bNext)); block->SetTrueTarget(bNext->GetTarget()); block->SetFalseTarget(bNext->Next()); diff --git a/src/coreclr/jit/fgprofilesynthesis.cpp b/src/coreclr/jit/fgprofilesynthesis.cpp index 923d327666498..1bf41de2dab92 100644 --- a/src/coreclr/jit/fgprofilesynthesis.cpp +++ b/src/coreclr/jit/fgprofilesynthesis.cpp @@ -335,9 +335,12 @@ void ProfileSynthesis::AssignLikelihoodSwitch(BasicBlock* block) { // Assume each switch case is equally probable // + const unsigned n = block->NumSucc(); assert(n != 0); - const weight_t p = 1 / (weight_t)n; + + // Duplicate zero check to silence "divide by zero" compiler warning + const weight_t p = (n != 0) ? (1 / (weight_t)n) : 0; // Each unique edge gets some multiple of that basic probability // diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index f1bdeeb22265f..7242380cb605b 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/lower.cpp b/src/coreclr/jit/lower.cpp index 24008029fec9d..dd5fc6b3673ba 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(false, false); modified |= comp->fgRemoveDeadBlocks(); if (modified) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 30b3fb845b634..e26af00a4a504 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -4648,7 +4648,7 @@ PhaseStatus Compiler::optOptimizeLayout() { noway_assert(opts.OptimizationEnabled()); - fgUpdateFlowGraph(/* doTailDuplication */ false); + fgUpdateFlowGraph(); fgReorderBlocks(/* useProfile */ true); fgUpdateFlowGraph(); @@ -5034,47 +5034,29 @@ 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)) + // For BBJ_COND blocks preceding header, fgReplaceJumpTarget set their false targets to preheader. + // Direct fallthrough to preheader by inserting a jump after the block. + // TODO-NoFallThrough: Remove this, and just rely on fgReplaceJumpTarget to do the fixup + BasicBlock* fallthroughSource = header->Prev(); + if (fallthroughSource->KindIs(BBJ_COND) && fallthroughSource->FalseTargetIs(preheader)) { - 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); + FlowEdge* edge = fgRemoveRefPred(preheader, fallthroughSource); + BasicBlock* newAlways = fgNewBBafter(BBJ_ALWAYS, fallthroughSource, true, preheader); + fgAddRefPred(preheader, newAlways, edge); + fgAddRefPred(newAlways, fallthroughSource, edge); - JITDUMP("Created new BBJ_ALWAYS " FMT_BB " to redirect " FMT_BB " over preheader\n", newAlways->bbNum, - fallthroughSource->bbNum); - } + JITDUMP("Created new BBJ_ALWAYS " FMT_BB " to redirect " FMT_BB " to preheader\n", newAlways->bbNum, + fallthroughSource->bbNum); + fallthroughSource->SetFalseTarget(newAlways); + } - fallthroughSource->SetFalseTarget(fallthroughSource->Next()); + // Make sure false target is set correctly for fallthrough into preheader. + if (!preheader->IsFirst()) + { + fallthroughSource = preheader->Prev(); + assert(!fallthroughSource->KindIs(BBJ_COND) || fallthroughSource->FalseTargetIs(preheader)); } optSetPreheaderWeight(loop, preheader); diff --git a/src/coreclr/jit/stacklevelsetter.cpp b/src/coreclr/jit/stacklevelsetter.cpp index b6aabca83e32d..4f37e1621e6fd 100644 --- a/src/coreclr/jit/stacklevelsetter.cpp +++ b/src/coreclr/jit/stacklevelsetter.cpp @@ -81,6 +81,29 @@ PhaseStatus StackLevelSetter::DoPhase() } } + // The above loop might have moved a BBJ_COND's true target to its next block. + // In such cases, reverse the condition so we can remove a branch. + if (madeChanges) + { + for (BasicBlock* const block : comp->Blocks()) + { + if (block->KindIs(BBJ_COND) && block->CanRemoveJumpToTarget(block->GetTrueTarget(), comp)) + { + // Reverse the jump condition + GenTree* test = block->lastNode(); + assert(test->OperIsConditionalJump()); + GenTree* cond = comp->gtReverseCond(test); + assert(cond == test); // Ensure `gtReverseCond` did not create a new node. + + BasicBlock* newFalseTarget = block->GetTrueTarget(); + BasicBlock* newTrueTarget = block->GetFalseTarget(); + block->SetTrueTarget(newTrueTarget); + block->SetFalseTarget(newFalseTarget); + assert(block->CanRemoveJumpToTarget(newFalseTarget, comp)); + } + } + } + return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } diff --git a/src/coreclr/jit/switchrecognition.cpp b/src/coreclr/jit/switchrecognition.cpp index 7ab33d07c3c34..fa6abd0f23e8b 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++) {