From 5019a4699c6ce8510509b8d7db788d9b71a683f6 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 3 Jul 2025 12:31:46 -0400 Subject: [PATCH 1/5] BBehfDesc -> BBJumpTable --- src/coreclr/jit/async.cpp | 7 +- src/coreclr/jit/block.cpp | 38 ++++------- src/coreclr/jit/block.h | 109 +++++++++++++++++++++---------- src/coreclr/jit/clrjit.natvis | 2 +- src/coreclr/jit/compiler.hpp | 8 +-- src/coreclr/jit/fgbasic.cpp | 88 +++++++------------------ src/coreclr/jit/fgdiagnostic.cpp | 9 +-- src/coreclr/jit/fgehopt.cpp | 2 +- src/coreclr/jit/fgflow.cpp | 6 +- src/coreclr/jit/importer.cpp | 27 ++++---- src/coreclr/jit/optimizer.cpp | 13 ++-- 11 files changed, 147 insertions(+), 162 deletions(-) diff --git a/src/coreclr/jit/async.cpp b/src/coreclr/jit/async.cpp index e72f1fe7007afa..5640d2dc51d3bb 100644 --- a/src/coreclr/jit/async.cpp +++ b/src/coreclr/jit/async.cpp @@ -290,12 +290,11 @@ BasicBlock* Compiler::InsertTryFinallyForContextRestore(BasicBlock* block, State block->SetTargetEdge(fgAddRefPred(callFinally, block)); callFinally->SetTargetEdge(fgAddRefPred(finallyRet, callFinally)); - BBehfDesc* ehfDesc = new (this, CMK_BasicBlock) BBehfDesc; - ehfDesc->bbeCount = 1; - ehfDesc->bbeSuccs = new (this, CMK_BasicBlock) FlowEdge* [1] { + FlowEdge** succs = new (this, CMK_BasicBlock) FlowEdge* [1] { fgAddRefPred(callFinallyRet, finallyRet) }; - ehfDesc->bbeSuccs[0]->setLikelihood(1.0); + succs[0]->setLikelihood(1.0); + BBJumpTable* ehfDesc = new (this, CMK_BasicBlock) BBJumpTable(succs, 1); finallyRet->SetEhfTargets(ehfDesc); callFinallyRet->SetTargetEdge(fgAddRefPred(goToTailBlock, callFinallyRet)); diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 6a3bc744bcda73..91dd6c097595cb 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -675,12 +675,9 @@ void BasicBlock::dspKind() const } else { - const unsigned jumpCnt = bbEhfTargets->bbeCount; - FlowEdge** const jumpTab = bbEhfTargets->bbeSuccs; - - for (unsigned i = 0; i < jumpCnt; i++) + for (unsigned i = 0; i < bbEhfTargets->GetSuccCount(); i++) { - printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i])); + printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(bbEhfTargets->GetSucc(i))); } } @@ -1187,7 +1184,7 @@ unsigned BasicBlock::NumSucc() const return 0; } - return bbEhfTargets->bbeCount; + return bbEhfTargets->GetSuccCount(); case BBJ_SWITCH: return bbSwtTargets->bbsCount; @@ -1232,7 +1229,7 @@ FlowEdge* BasicBlock::GetSuccEdge(unsigned i) const } case BBJ_EHFINALLYRET: - return bbEhfTargets->bbeSuccs[i]; + return bbEhfTargets->GetSucc(i); case BBJ_SWITCH: return bbSwtTargets->bbsDstTab[i]; @@ -1291,7 +1288,7 @@ unsigned BasicBlock::NumSucc(Compiler* comp) return 0; } - return bbEhfTargets->bbeCount; + return bbEhfTargets->GetSuccCount(); case BBJ_CALLFINALLY: case BBJ_CALLFINALLYRET: @@ -1346,8 +1343,7 @@ FlowEdge* BasicBlock::GetSuccEdge(unsigned i, Compiler* comp) case BBJ_EHFINALLYRET: assert(bbEhfTargets != nullptr); - assert(i < bbEhfTargets->bbeCount); - return bbEhfTargets->bbeSuccs[i]; + return bbEhfTargets->GetSucc(i); case BBJ_CALLFINALLY: case BBJ_CALLFINALLYRET: @@ -1639,7 +1635,7 @@ BasicBlock* BasicBlock::New(Compiler* compiler, BBKinds kind) return block; } -BasicBlock* BasicBlock::New(Compiler* compiler, BBehfDesc* ehfTargets) +BasicBlock* BasicBlock::New(Compiler* compiler, BBJumpTable* ehfTargets) { BasicBlock* block = BasicBlock::New(compiler); block->SetEhf(ehfTargets); @@ -1793,29 +1789,23 @@ BBswtDesc::BBswtDesc(Compiler* comp, const BBswtDesc* other) // Allocate and fill in a new dst tab // bbsDstTab = new (comp, CMK_FlowEdge) FlowEdge*[bbsCount]; - for (unsigned i = 0; i < bbsCount; i++) - { - bbsDstTab[i] = other->bbsDstTab[i]; - } + memcpy(bbsDstTab, other->bbsDstTab, sizeof(FlowEdge*) * bbsCount); } //------------------------------------------------------------------------ -// BBehfDesc copy ctor: copy a EHFINALLYRET descriptor +// BBJumpTable copy ctor: copy a N-successor block descriptor // // Arguments: // comp - compiler instance // other - existing descriptor to copy // -BBehfDesc::BBehfDesc(Compiler* comp, const BBehfDesc* other) - : bbeCount(other->bbeCount) +BBJumpTable::BBJumpTable(Compiler* comp, const BBJumpTable* other) + : succs(new(comp, CMK_FlowEdge) FlowEdge*[other->succCount]) + , succCount(other->succCount) { - // Allocate and fill in a new dst tab + // Fill in the new jump table // - bbeSuccs = new (comp, CMK_FlowEdge) FlowEdge*[bbeCount]; - for (unsigned i = 0; i < bbeCount; i++) - { - bbeSuccs[i] = other->bbeSuccs[i]; - } + memcpy(succs, other->succs, sizeof(FlowEdge*) * succCount); } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 3bb90fba0a5256..b1f5e7423ec9cc 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -107,7 +107,7 @@ struct BasicBlockList; struct FlowEdge; struct EHblkDsc; struct BBswtDesc; -struct BBehfDesc; +struct BBJumpTable; struct StackEntry { @@ -396,16 +396,16 @@ class BBSwitchTargetList BBArrayIterator end() const; }; -// BBEhfSuccList: adapter class for forward iteration of BBJ_EHFINALLYRET blocks, using range-based `for`, +// BBJumpTableList: adapter class for forward iteration of blocks with N successors, using range-based `for`, // normally used via BasicBlock::EHFinallyRetSuccs(), e.g.: // for (BasicBlock* const succ : block->EHFinallyRetSuccs()) ... // -class BBEhfSuccList +class BBJumpTableList { - BBehfDesc* m_bbeDesc; + BBJumpTable* m_bbJumpTable; public: - BBEhfSuccList(BBehfDesc* bbeDesc); + BBJumpTableList(BBJumpTable* bbJumpTable); BBArrayIterator begin() const; BBArrayIterator end() const; }; @@ -743,11 +743,11 @@ struct BasicBlock : private LIR::Range /* The following union describes the jump target(s) of this block */ union { - unsigned bbTargetOffs; // PC offset (temporary only) - FlowEdge* bbTargetEdge; // successor edge for block kinds with only one successor (BBJ_ALWAYS, etc) - FlowEdge* bbTrueEdge; // BBJ_COND successor edge when its condition is true (alias for bbTargetEdge) - BBswtDesc* bbSwtTargets; // switch descriptor - BBehfDesc* bbEhfTargets; // BBJ_EHFINALLYRET descriptor + unsigned bbTargetOffs; // PC offset (temporary only) + FlowEdge* bbTargetEdge; // successor edge for block kinds with only one successor (BBJ_ALWAYS, etc) + FlowEdge* bbTrueEdge; // BBJ_COND successor edge when its condition is true (alias for bbTargetEdge) + BBswtDesc* bbSwtTargets; // switch descriptor + BBJumpTable* bbEhfTargets; // BBJ_EHFINALLYRET descriptor }; // Successor edge of a BBJ_COND block if bbTrueEdge is not taken @@ -756,7 +756,7 @@ struct BasicBlock : private LIR::Range public: static BasicBlock* New(Compiler* compiler); static BasicBlock* New(Compiler* compiler, BBKinds kind); - static BasicBlock* New(Compiler* compiler, BBehfDesc* ehfTargets); + static BasicBlock* New(Compiler* compiler, BBJumpTable* ehfTargets); static BasicBlock* New(Compiler* compiler, BBswtDesc* swtTargets); static BasicBlock* New(Compiler* compiler, BBKinds kind, unsigned targetOffs); @@ -1032,19 +1032,19 @@ struct BasicBlock : private LIR::Range bbSwtTargets = swtTarget; } - BBehfDesc* GetEhfTargets() const + BBJumpTable* GetEhfTargets() const { assert(KindIs(BBJ_EHFINALLYRET)); return bbEhfTargets; } - void SetEhfTargets(BBehfDesc* ehfTarget) + void SetEhfTargets(BBJumpTable* ehfTarget) { assert(KindIs(BBJ_EHFINALLYRET)); bbEhfTargets = ehfTarget; } - void SetEhf(BBehfDesc* ehfTarget) + void SetEhf(BBJumpTable* ehfTarget) { assert(ehfTarget != nullptr); bbKind = BBJ_EHFINALLYRET; @@ -1421,10 +1421,10 @@ struct BasicBlock : private LIR::Range // successors, e.g.: // for (BasicBlock* const succ : block->EHFinallyRetSuccs()) ... // - BBEhfSuccList EHFinallyRetSuccs() const + BBJumpTableList EHFinallyRetSuccs() const { assert(bbKind == BBJ_EHFINALLYRET); - return BBEhfSuccList(bbEhfTargets); + return BBJumpTableList(bbEhfTargets); } BasicBlock* GetUniquePred(Compiler* comp) const; @@ -2351,40 +2351,79 @@ inline BBArrayIterator BBSwitchTargetList::end() const return BBArrayIterator(m_bbsDesc->bbsDstTab + m_bbsDesc->bbsCount); } -// BBehfDesc -- descriptor for a BBJ_EHFINALLYRET block +// BBJumpTable -- descriptor blocks with N successors // -struct BBehfDesc +struct BBJumpTable { - FlowEdge** bbeSuccs; // array of `FlowEdge*` pointing to BBJ_EHFINALLYRET block successors - unsigned bbeCount; // size of `bbeSuccs` array +private: + FlowEdge** const succs; // array of unique `FlowEdge*` pointing to the block's successors + unsigned succCount; // Number of unique successors + +public: + BBJumpTable() + : succs(nullptr) + , succCount(0) + { + } - BBehfDesc() - : bbeSuccs(nullptr) - , bbeCount(0) + BBJumpTable(FlowEdge** succs, unsigned succCount) + : succs(succs) + , succCount(succCount) { } - BBehfDesc(Compiler* comp, const BBehfDesc* other); + BBJumpTable(Compiler* comp, const BBJumpTable* other); + + FlowEdge** GetSuccs() const + { + return succs; + } + + FlowEdge* GetSucc(unsigned index) const + { + assert(index < succCount); + return succs[index]; + } + + unsigned GetSuccCount() const + { + return succCount; + } + + void RemoveSucc(unsigned index) + { + assert(index < succCount); + assert(succs != nullptr); + + // If succEdge is not the last entry, move everything after in the table down one slot. + if ((index + 1) < succCount) + { + memmove_s(&succs[index], (succCount - index) * sizeof(FlowEdge*), &succs[index + 1], + (succCount - index - 1) * sizeof(FlowEdge*)); + } + + succCount--; + } }; -// BBEhfSuccList out-of-class-declaration implementations (here due to C++ ordering requirements). +// BBJumpTableList out-of-class-declaration implementations (here due to C++ ordering requirements). // -inline BBEhfSuccList::BBEhfSuccList(BBehfDesc* bbeDesc) - : m_bbeDesc(bbeDesc) +inline BBJumpTableList::BBJumpTableList(BBJumpTable* bbJumpTable) + : m_bbJumpTable(bbJumpTable) { - assert(m_bbeDesc != nullptr); - assert((m_bbeDesc->bbeSuccs != nullptr) || (m_bbeDesc->bbeCount == 0)); + assert(m_bbJumpTable != nullptr); + assert((m_bbJumpTable->GetSuccs() != nullptr) || (m_bbJumpTable->GetSuccCount() == 0)); } -inline BBArrayIterator BBEhfSuccList::begin() const +inline BBArrayIterator BBJumpTableList::begin() const { - return BBArrayIterator(m_bbeDesc->bbeSuccs); + return BBArrayIterator(m_bbJumpTable->GetSuccs()); } -inline BBArrayIterator BBEhfSuccList::end() const +inline BBArrayIterator BBJumpTableList::end() const { - return BBArrayIterator(m_bbeDesc->bbeSuccs + m_bbeDesc->bbeCount); + return BBArrayIterator(m_bbJumpTable->GetSuccs() + m_bbJumpTable->GetSuccCount()); } // SuccList out-of-class-declaration implementations @@ -2442,8 +2481,8 @@ inline BasicBlock::SuccList::SuccList(const BasicBlock* block) } else { - m_begin = block->GetEhfTargets()->bbeSuccs; - m_end = block->GetEhfTargets()->bbeSuccs + block->GetEhfTargets()->bbeCount; + m_begin = block->GetEhfTargets()->GetSuccs(); + m_end = block->GetEhfTargets()->GetSuccs() + block->GetEhfTargets()->GetSuccCount(); } break; diff --git a/src/coreclr/jit/clrjit.natvis b/src/coreclr/jit/clrjit.natvis index 424bd4e3f2dca0..c87546a448596a 100644 --- a/src/coreclr/jit/clrjit.natvis +++ b/src/coreclr/jit/clrjit.natvis @@ -23,7 +23,7 @@ Documentation for VS debugger format specifiers: https://learn.microsoft.com/vis BB{bbNum,d}->BB{bbTargetEdge->m_destBlock->bbNum,d}; {bbKind,en} BB{bbNum,d}-> (BB{bbTrueEdge->m_destBlock->bbNum,d}(T),BB{bbFalseEdge->m_destBlock->bbNum,d}(F)) ; {bbKind,en} BB{bbNum,d}; {bbKind,en}; {bbSwtTargets->bbsCount} cases - BB{bbNum,d}; {bbKind,en}; {bbEhfTargets->bbeCount} succs + BB{bbNum,d}; {bbKind,en}; {bbEhfTargets->succCount} succs BB{bbNum,d}; {bbKind,en} diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 0f9b1de014ad49..3560d0e1632fa0 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -663,9 +663,9 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func, const bool // LEAVE into callfinally yet, and haven't added return successors. if (bbEhfTargets != nullptr) { - for (unsigned i = 0; i < bbEhfTargets->bbeCount; i++) + for (unsigned i = 0; i < bbEhfTargets->GetSuccCount(); i++) { - RETURN_ON_ABORT(func(bbEhfTargets->bbeSuccs[i]->getDestinationBlock())); + RETURN_ON_ABORT(func(bbEhfTargets->GetSucc(i)->getDestinationBlock())); } } @@ -750,9 +750,9 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func) // LEAVE into callfinally yet, and haven't added return successors. if (bbEhfTargets != nullptr) { - for (unsigned i = 0; i < bbEhfTargets->bbeCount; i++) + for (unsigned i = 0; i < bbEhfTargets->GetSuccCount(); i++) { - RETURN_ON_ABORT(func(bbEhfTargets->bbeSuccs[i]->getDestinationBlock())); + RETURN_ON_ABORT(func(bbEhfTargets->GetSucc(i)->getDestinationBlock())); } } diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 4fa17a9c6c8207..9815fccb9c04ba 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -259,11 +259,11 @@ void Compiler::fgChangeEhfBlock(BasicBlock* oldBlock, BasicBlock* newBlock) assert(oldBlock->KindIs(BBJ_EHFINALLYRET)); assert(fgPredsComputed); - BBehfDesc* ehfDesc = oldBlock->GetEhfTargets(); + BBJumpTable* ehfDesc = oldBlock->GetEhfTargets(); - for (unsigned i = 0; i < ehfDesc->bbeCount; i++) + for (unsigned i = 0; i < ehfDesc->GetSuccCount(); i++) { - FlowEdge* succEdge = ehfDesc->bbeSuccs[i]; + FlowEdge* succEdge = ehfDesc->GetSucc(i); fgReplacePred(succEdge, newBlock); } } @@ -288,9 +288,9 @@ void Compiler::fgReplaceEhfSuccessor(BasicBlock* block, BasicBlock* oldSucc, Bas assert(block->KindIs(BBJ_EHFINALLYRET)); assert(fgPredsComputed); - BBehfDesc* const ehfDesc = block->GetEhfTargets(); - const unsigned succCount = ehfDesc->bbeCount; - FlowEdge** const succTab = ehfDesc->bbeSuccs; + BBJumpTable* const ehfDesc = block->GetEhfTargets(); + const unsigned succCount = ehfDesc->GetSuccCount(); + FlowEdge** const succTab = ehfDesc->GetSuccs(); // Walk the successor table looking for the old successor, which we expect to find only once. unsigned oldSuccNum = UINT_MAX; @@ -344,48 +344,38 @@ void Compiler::fgReplaceEhfSuccessor(BasicBlock* block, BasicBlock* oldSucc, Bas // // Arguments: // block - BBJ_EHFINALLYRET block -// succIndex - index of the successor in block->GetEhfTargets()->bbeSuccs +// succIndex - index of the successor in the block's jump table // void Compiler::fgRemoveEhfSuccFromTable(BasicBlock* block, const unsigned succIndex) { assert(block != nullptr); assert(block->KindIs(BBJ_EHFINALLYRET)); - BBehfDesc* const ehfDesc = block->GetEhfTargets(); - const unsigned succCount = ehfDesc->bbeCount; - FlowEdge** succTab = ehfDesc->bbeSuccs; - assert(succIndex < succCount); - FlowEdge* const succEdge = succTab[succIndex]; - - // If succEdge is not the last entry, move everything after in the table down one slot. - if ((succIndex + 1) < succCount) - { - memmove_s(&succTab[succIndex], (succCount - succIndex) * sizeof(FlowEdge*), &succTab[succIndex + 1], - (succCount - succIndex - 1) * sizeof(FlowEdge*)); - } + BBJumpTable* const ehfDesc = block->GetEhfTargets(); + FlowEdge* const succEdge = ehfDesc->GetSucc(succIndex); + ehfDesc->RemoveSucc(succIndex); // Recompute the likelihoods of the block's other successor edges. const weight_t removedLikelihood = succEdge->getLikelihood(); - const unsigned newSuccCount = succCount - 1; + const unsigned newSuccCount = ehfDesc->GetSuccCount(); for (unsigned i = 0; i < newSuccCount; i++) { // If we removed all of the flow out of 'block', distribute flow among the remaining edges evenly. - const weight_t currLikelihood = succTab[i]->getLikelihood(); - const weight_t newLikelihood = + FlowEdge* const edge = ehfDesc->GetSucc(i); + const weight_t currLikelihood = edge->getLikelihood(); + const weight_t newLikelihood = (removedLikelihood == 1.0) ? (1.0 / newSuccCount) : (currLikelihood / (1.0 - removedLikelihood)); - succTab[i]->setLikelihood(min(1.0, newLikelihood)); + edge->setLikelihood(min(1.0, newLikelihood)); } #ifdef DEBUG // We only expect to see a successor once in the table. - for (unsigned i = succIndex; i < (succCount - 1); i++) + for (unsigned i = succIndex; i < newSuccCount; i++) { - assert(succTab[i]->getDestinationBlock() != succEdge->getDestinationBlock()); + assert(ehfDesc->GetSucc(i)->getDestinationBlock() != succEdge->getDestinationBlock()); } #endif // DEBUG - - ehfDesc->bbeCount--; } //------------------------------------------------------------------------ @@ -407,50 +397,18 @@ void Compiler::fgRemoveEhfSuccessor(FlowEdge* succEdge) fgRemoveRefPred(succEdge); - BBehfDesc* const ehfDesc = block->GetEhfTargets(); - const unsigned succCount = ehfDesc->bbeCount; - FlowEdge** succTab = ehfDesc->bbeSuccs; - bool found = false; + BBJumpTable* const ehfDesc = block->GetEhfTargets(); - // Search succTab for succEdge so we can splice it out of the table. - for (unsigned i = 0; i < succCount; i++) + for (unsigned i = 0; i < ehfDesc->GetSuccCount(); i++) { - if (succTab[i] == succEdge) + if (ehfDesc->GetSucc(i) == succEdge) { - // If succEdge not the last entry, move everything after in the table down one slot. - if ((i + 1) < succCount) - { - memmove_s(&succTab[i], (succCount - i) * sizeof(FlowEdge*), &succTab[i + 1], - (succCount - i - 1) * sizeof(FlowEdge*)); - } - - found = true; - -#ifdef DEBUG - // We only expect to see a successor once in the table. - for (; i < (succCount - 1); i++) - { - assert(succTab[i]->getDestinationBlock() != succEdge->getDestinationBlock()); - } -#endif // DEBUG + fgRemoveEhfSuccFromTable(block, i); + return; } } - // Recompute the likelihoods of the block's other successor edges. - const weight_t removedLikelihood = succEdge->getLikelihood(); - const unsigned newSuccCount = succCount - 1; - - for (unsigned i = 0; i < newSuccCount; i++) - { - // If we removed all of the flow out of 'block', distribute flow among the remaining edges evenly. - const weight_t currLikelihood = succTab[i]->getLikelihood(); - const weight_t newLikelihood = - (removedLikelihood == 1.0) ? (1.0 / newSuccCount) : (currLikelihood / (1.0 - removedLikelihood)); - succTab[i]->setLikelihood(min(1.0, newLikelihood)); - } - - assert(found); - ehfDesc->bbeCount--; + unreached(); } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 50145f04b32b81..fbd28bec04e4cd 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -1978,7 +1978,7 @@ void Compiler::fgTableDispBasicBlock(const BasicBlock* block, printf("->"); printedBlockWidth = 2 + 9 /* kind */; - const BBehfDesc* const ehfDesc = block->GetEhfTargets(); + const BBJumpTable* const ehfDesc = block->GetEhfTargets(); if (ehfDesc == nullptr) { printf(" ????"); @@ -1988,13 +1988,10 @@ void Compiler::fgTableDispBasicBlock(const BasicBlock* block, { // Very early in compilation, we won't have fixed up the BBJ_EHFINALLYRET successors yet. - const unsigned jumpCnt = ehfDesc->bbeCount; - FlowEdge** const jumpTab = ehfDesc->bbeSuccs; - - for (unsigned i = 0; i < jumpCnt; i++) + for (unsigned i = 0; i < ehfDesc->GetSuccCount(); i++) { printedBlockWidth += 1 /* space/comma */; - printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i])); + printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(ehfDesc->GetSucc(i))); } } diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 084afc5846813d..ceb5d7c12a236d 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -1607,7 +1607,7 @@ PhaseStatus Compiler::fgCloneFinally() { if (block->KindIs(BBJ_EHFINALLYRET)) { - assert(block->GetEhfTargets()->bbeCount == 0); + assert(block->GetEhfTargets()->GetSuccCount() == 0); block->SetKind(BBJ_EHFAULTRET); } } diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index f7a055f4a76e57..240252a4558a4c 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -307,10 +307,10 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block) case BBJ_EHFINALLYRET: { - BBehfDesc* const ehfDesc = block->GetEhfTargets(); - for (unsigned i = 0; i < ehfDesc->bbeCount; i++) + BBJumpTable* const ehfDesc = block->GetEhfTargets(); + for (unsigned i = 0; i < ehfDesc->GetSuccCount(); i++) { - fgRemoveRefPred(ehfDesc->bbeSuccs[i]); + fgRemoveAllRefPreds(ehfDesc->GetSucc(i)->getDestinationBlock(), block); } break; } diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 21f12c21e0dc59..bb012edec025f5 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -12793,19 +12793,13 @@ void Compiler::impFixPredLists() } } - BBehfDesc* jumpEhf = new (this, CMK_BasicBlock) BBehfDesc; + BBJumpTable* jumpEhf; - // It's possible for the `finally` to have no CALLFINALLY predecessors if the `try` block - // has an unconditional `throw` (the finally will still be invoked in the exceptional - // case via the runtime). In that case, jumpEhf->bbeCount remains the default, zero, - // and jumpEhf->bbeSuccs remains the default, nullptr. if (predCount > 0) { - jumpEhf->bbeCount = predCount; - jumpEhf->bbeSuccs = new (this, CMK_FlowEdge) FlowEdge*[predCount]; - - unsigned predNum = 0; - weight_t remainingLikelihood = 1.0; + FlowEdge** const succTab = new (this, CMK_FlowEdge) FlowEdge*[predCount]; + unsigned predNum = 0; + weight_t remainingLikelihood = 1.0; for (BasicBlock* const predBlock : finallyBegBlock->PredBlocks()) { // We only care about preds that are callfinallies. @@ -12834,8 +12828,7 @@ void Compiler::impFixPredLists() newEdge->setLikelihood(1.0 / predCount); } - jumpEhf->bbeSuccs[predNum] = newEdge; - ++predNum; + succTab[predNum++] = newEdge; if (!added) { @@ -12843,7 +12836,17 @@ void Compiler::impFixPredLists() added = true; } } + assert(predNum == predCount); + jumpEhf = new (this, CMK_FlowEdge) BBJumpTable(succTab, predCount); + } + else + { + // It's possible for the `finally` to have no CALLFINALLY predecessors if the `try` block + // has an unconditional `throw` (the finally will still be invoked in the exceptional + // case via the runtime). In that case, jumpEhf->succCount remains the default, zero, + // and jumpEhf->succs remains the default, nullptr. + jumpEhf = new (this, CMK_FlowEdge) BBJumpTable(); } finallyBlock->SetEhfTargets(jumpEhf); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 6399e26d3fc16c..a2bd4dd7d7c833 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -585,14 +585,12 @@ void Compiler::optSetMappedBlockTargets(BasicBlock* blk, BasicBlock* newBlk, Blo case BBJ_EHFINALLYRET: { - BBehfDesc* currEhfDesc = blk->GetEhfTargets(); - BBehfDesc* newEhfDesc = new (this, CMK_BasicBlock) BBehfDesc; - newEhfDesc->bbeCount = currEhfDesc->bbeCount; - newEhfDesc->bbeSuccs = new (this, CMK_FlowEdge) FlowEdge*[newEhfDesc->bbeCount]; + BBJumpTable* currEhfDesc = blk->GetEhfTargets(); + FlowEdge** newSuccs = new (this, CMK_FlowEdge) FlowEdge*[currEhfDesc->GetSuccCount()]; - for (unsigned i = 0; i < newEhfDesc->bbeCount; i++) + for (unsigned i = 0; i < currEhfDesc->GetSuccCount(); i++) { - FlowEdge* const inspiringEdge = currEhfDesc->bbeSuccs[i]; + FlowEdge* const inspiringEdge = currEhfDesc->GetSucc(i); BasicBlock* const ehfTarget = inspiringEdge->getDestinationBlock(); FlowEdge* newEdge; @@ -606,9 +604,10 @@ void Compiler::optSetMappedBlockTargets(BasicBlock* blk, BasicBlock* newBlk, Blo newEdge = fgAddRefPred(ehfTarget, newBlk, inspiringEdge); } - newEhfDesc->bbeSuccs[i] = newEdge; + newSuccs[i] = newEdge; } + BBJumpTable* newEhfDesc = new (this, CMK_BasicBlock) BBJumpTable(newSuccs, currEhfDesc->GetSuccCount()); newBlk->SetEhf(newEhfDesc); break; } From c334fe29938320823f955b2eb6afa3962bf52d5f Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 8 Jul 2025 11:17:01 -0400 Subject: [PATCH 2/5] BBswtDesc refactor --- src/coreclr/jit/assertionprop.cpp | 15 +- src/coreclr/jit/async.cpp | 24 ++- src/coreclr/jit/block.cpp | 84 ++------- src/coreclr/jit/block.h | 246 +++++++++++++++++--------- src/coreclr/jit/clrjit.natvis | 2 +- src/coreclr/jit/codegencommon.cpp | 13 +- src/coreclr/jit/compiler.h | 39 ---- src/coreclr/jit/compiler.hpp | 10 +- src/coreclr/jit/fgbasic.cpp | 124 +++++-------- src/coreclr/jit/fgdiagnostic.cpp | 84 +++------ src/coreclr/jit/fgflow.cpp | 143 +-------------- src/coreclr/jit/fgopt.cpp | 97 ++++------ src/coreclr/jit/fgprofile.cpp | 9 +- src/coreclr/jit/importer.cpp | 20 +-- src/coreclr/jit/lower.cpp | 93 ++++------ src/coreclr/jit/morph.cpp | 4 +- src/coreclr/jit/optimizer.cpp | 13 +- src/coreclr/jit/switchrecognition.cpp | 31 ++-- 18 files changed, 405 insertions(+), 646 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index b63ea4084e11d8..f11f7623d9bc73 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -5630,8 +5630,8 @@ bool Compiler::optCreateJumpTableImpliedAssertions(BasicBlock* switchBb) GenTree* switchTree = switchBb->lastStmt()->GetRootNode()->gtEffectiveVal(); assert(switchTree->OperIs(GT_SWITCH)); - // bbsCount is uint32_t, but it's unlikely to be more than INT32_MAX. - noway_assert(switchBb->GetSwitchTargets()->bbsCount <= INT32_MAX); + // Case count is uint32_t, but it's unlikely to be more than INT32_MAX. + noway_assert(switchBb->GetSwitchTargets()->GetCaseCount() <= INT32_MAX); ValueNum opVN = optConservativeNormalVN(switchTree->gtGetOp1()); if (opVN == ValueNumStore::NoVN) @@ -5649,9 +5649,9 @@ bool Compiler::optCreateJumpTableImpliedAssertions(BasicBlock* switchBb) int offset = 0; vnStore->PeelOffsetsI32(&opVN, &offset); - int jumpCount = static_cast(switchBb->GetSwitchTargets()->bbsCount); - FlowEdge** jumpTable = switchBb->GetSwitchTargets()->bbsDstTab; - bool hasDefault = switchBb->GetSwitchTargets()->bbsHasDefault; + int jumpCount = static_cast(switchBb->GetSwitchTargets()->GetCaseCount()); + FlowEdge** jumpTable = switchBb->GetSwitchTargets()->GetCases(); + bool hasDefault = switchBb->GetSwitchTargets()->HasDefaultCase(); for (int jmpTargetIdx = 0; jmpTargetIdx < jumpCount; jmpTargetIdx++) { @@ -5663,14 +5663,15 @@ bool Compiler::optCreateJumpTableImpliedAssertions(BasicBlock* switchBb) int value = jmpTargetIdx - offset; // We can only make "X == caseValue" assertions for blocks with a single edge from the switch. - BasicBlock* target = jumpTable[jmpTargetIdx]->getDestinationBlock(); + FlowEdge* const edge = jumpTable[jmpTargetIdx]; + BasicBlock* const target = edge->getDestinationBlock(); if (target->GetUniquePred(this) != switchBb) { // Target block is potentially reachable from multiple blocks (outside the switch). continue; } - if (fgGetPredForBlock(target, switchBb)->getDupCount() > 1) + if (edge->getDupCount() > 1) { // We have just one predecessor (BBJ_SWITCH), but there may be multiple edges (cases) per target. continue; diff --git a/src/coreclr/jit/async.cpp b/src/coreclr/jit/async.cpp index 5640d2dc51d3bb..28151906f2e6bc 100644 --- a/src/coreclr/jit/async.cpp +++ b/src/coreclr/jit/async.cpp @@ -2232,18 +2232,26 @@ void AsyncTransformation::CreateResumptionSwitch() // Default case. TODO-CQ: Support bbsHasDefault = false before lowering. m_resumptionBBs.push_back(m_resumptionBBs[0]); - BBswtDesc* swtDesc = new (m_comp, CMK_BasicBlock) BBswtDesc; - swtDesc->bbsCount = (unsigned)m_resumptionBBs.size(); - swtDesc->bbsHasDefault = true; - swtDesc->bbsDstTab = new (m_comp, CMK_Async) FlowEdge*[m_resumptionBBs.size()]; + const size_t numCases = m_resumptionBBs.size(); + FlowEdge** const cases = new (m_comp, CMK_FlowEdge) FlowEdge*[numCases * 2]; + FlowEdge** const succs = cases + numCases; + unsigned numUniqueSuccs = 0; - weight_t stateLikelihood = 1.0 / m_resumptionBBs.size(); - for (size_t i = 0; i < m_resumptionBBs.size(); i++) + const weight_t stateLikelihood = 1.0 / m_resumptionBBs.size(); + for (size_t i = 0; i < numCases; i++) { - swtDesc->bbsDstTab[i] = m_comp->fgAddRefPred(m_resumptionBBs[i], switchBB); - swtDesc->bbsDstTab[i]->setLikelihood(stateLikelihood); + FlowEdge* const edge = m_comp->fgAddRefPred(m_resumptionBBs[i], switchBB); + edge->setLikelihood(stateLikelihood); + cases[i] = edge; + + if (edge->getDupCount() == 1) + { + succs[numUniqueSuccs++] = edge; + } } + BBswtDesc* const swtDesc = + new (m_comp, CMK_BasicBlock) BBswtDesc(cases, (unsigned)numCases, succs, numUniqueSuccs, true); switchBB->SetSwitch(swtDesc); } diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 91dd6c097595cb..6187216d0d0706 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -589,39 +589,10 @@ unsigned BasicBlock::dspPreds() const void BasicBlock::dspSuccs(Compiler* compiler) { bool first = true; - - // If this is a switch, we don't want to call `Succs(Compiler*)` because it will eventually call - // `GetSwitchDescMap()`, and that will have the side-effect of allocating the unique switch descriptor map - // and/or compute this switch block's unique succ set if it is not present. Debug output functions should - // never have an effect on codegen. We also don't want to assume the unique succ set is accurate, so we - // compute it ourselves here. - if (bbKind == BBJ_SWITCH) - { - // Create a set with all the successors. - unsigned bbNumMax = compiler->fgBBNumMax; - BitVecTraits bitVecTraits(bbNumMax + 1, compiler); - BitVec uniqueSuccBlocks(BitVecOps::MakeEmpty(&bitVecTraits)); - for (BasicBlock* const bTarget : SwitchTargets()) - { - BitVecOps::AddElemD(&bitVecTraits, uniqueSuccBlocks, bTarget->bbNum); - } - BitVecOps::Iter iter(&bitVecTraits, uniqueSuccBlocks); - unsigned bbNum = 0; - while (iter.NextElem(&bbNum)) - { - // Note that we will output switch successors in increasing numerical bbNum order, which is - // not related to their order in the bbSwtTargets->bbsDstTab table. - printf("%s" FMT_BB, first ? "" : ",", bbNum); - first = false; - } - } - else + for (const BasicBlock* const succ : Succs(compiler)) { - for (const BasicBlock* const succ : Succs(compiler)) - { - printf("%s" FMT_BB, first ? "" : ",", succ->bbNum); - first = false; - } + printf("%s" FMT_BB, first ? "" : ",", succ->bbNum); + first = false; } } @@ -736,20 +707,20 @@ void BasicBlock::dspKind() const { printf(" ->"); - const unsigned jumpCnt = bbSwtTargets->bbsCount; - FlowEdge** const jumpTab = bbSwtTargets->bbsDstTab; + const unsigned jumpCnt = bbSwtTargets->GetCaseCount(); + FlowEdge** const jumpTab = bbSwtTargets->GetCases(); for (unsigned i = 0; i < jumpCnt; i++) { printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i])); - const bool isDefault = bbSwtTargets->bbsHasDefault && (i == jumpCnt - 1); + const bool isDefault = bbSwtTargets->HasDefaultCase() && (i == jumpCnt - 1); if (isDefault) { printf("[def]"); } - const bool isDominant = bbSwtTargets->bbsHasDominantCase && (i == bbSwtTargets->bbsDominantCase); + const bool isDominant = bbSwtTargets->HasDominantCase() && (i == bbSwtTargets->GetDominantCase()); if (isDominant) { printf("[dom]"); @@ -1187,7 +1158,7 @@ unsigned BasicBlock::NumSucc() const return bbEhfTargets->GetSuccCount(); case BBJ_SWITCH: - return bbSwtTargets->bbsCount; + return bbSwtTargets->GetCaseCount(); default: unreached(); @@ -1232,7 +1203,7 @@ FlowEdge* BasicBlock::GetSuccEdge(unsigned i) const return bbEhfTargets->GetSucc(i); case BBJ_SWITCH: - return bbSwtTargets->bbsDstTab[i]; + return bbSwtTargets->GetCase(i); default: unreached(); @@ -1309,10 +1280,7 @@ unsigned BasicBlock::NumSucc(Compiler* comp) } case BBJ_SWITCH: - { - Compiler::SwitchUniqueSuccSet sd = comp->GetDescriptorForSwitch(this); - return sd.numDistinctSuccs; - } + return bbSwtTargets->GetSuccCount(); default: unreached(); @@ -1365,11 +1333,7 @@ FlowEdge* BasicBlock::GetSuccEdge(unsigned i, Compiler* comp) } case BBJ_SWITCH: - { - Compiler::SwitchUniqueSuccSet sd = comp->GetDescriptorForSwitch(this); - assert(i < sd.numDistinctSuccs); // Range check. - return sd.nonDuplicates[i]; - } + return bbSwtTargets->GetSucc(i); default: unreached(); @@ -1757,21 +1721,6 @@ bool BasicBlock::hasEHBoundaryOut() const return KindIs(BBJ_EHFILTERRET, BBJ_EHFINALLYRET, BBJ_EHFAULTRET, BBJ_EHCATCHRET); } -//------------------------------------------------------------------------ -// BBswtDesc copy ctor: copy a switch descriptor, but don't set up the jump table -// -// Arguments: -// other - existing switch descriptor to copy (except for its jump table) -// -BBswtDesc::BBswtDesc(const BBswtDesc* other) - : bbsDstTab(nullptr) - , bbsCount(other->bbsCount) - , bbsDominantCase(other->bbsDominantCase) - , bbsHasDefault(other->bbsHasDefault) - , bbsHasDominantCase(other->bbsHasDominantCase) -{ -} - //------------------------------------------------------------------------ // BBswtDesc copy ctor: copy a switch descriptor // @@ -1780,16 +1729,17 @@ BBswtDesc::BBswtDesc(const BBswtDesc* other) // other - existing switch descriptor to copy // BBswtDesc::BBswtDesc(Compiler* comp, const BBswtDesc* other) - : bbsDstTab(nullptr) - , bbsCount(other->bbsCount) + : BBJumpTable(new(comp, CMK_FlowEdge) FlowEdge*[other->succCount + other->caseCount], other -> succCount) + , caseCount(other->caseCount) + , cases(succs + succCount) , bbsDominantCase(other->bbsDominantCase) , bbsHasDefault(other->bbsHasDefault) , bbsHasDominantCase(other->bbsHasDominantCase) { - // Allocate and fill in a new dst tab + // Fill in the new tables // - bbsDstTab = new (comp, CMK_FlowEdge) FlowEdge*[bbsCount]; - memcpy(bbsDstTab, other->bbsDstTab, sizeof(FlowEdge*) * bbsCount); + memcpy(succs, other->succs, sizeof(FlowEdge*) * succCount); + memcpy(cases, other->cases, sizeof(FlowEdge*) * caseCount); } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index b1f5e7423ec9cc..66f6cc319f71bb 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -346,7 +346,7 @@ class BBArrayIterator } }; -// FlowEdgeArrayIterator: forward iterator for an array of FlowEdge*, such as the BBswtDesc->bbsDstTab. +// FlowEdgeArrayIterator: forward iterator for an array of FlowEdge*, such as BBJumpTable::succs. // It is an error (with assert) to yield a nullptr FlowEdge* in this array. // `m_edgeEntry` can be nullptr, but it only makes sense if both the begin and end of an iteration range are nullptr // (meaning, no actual iteration will happen). @@ -383,8 +383,8 @@ class FlowEdgeArrayIterator }; // BBSwitchTargetList: adapter class for forward iteration of switch targets, using range-based `for`, -// normally used via BasicBlock::SwitchTargets(), e.g.: -// for (BasicBlock* const target : block->SwitchTargets()) ... +// normally used via BasicBlock::SwitchCases(), e.g.: +// for (BasicBlock* const target : block->SwitchCases()) ... // class BBSwitchTargetList { @@ -1408,15 +1408,25 @@ struct BasicBlock : private LIR::Range BasicBlock* GetSucc(unsigned i) const; BasicBlock* GetSucc(unsigned i, Compiler* comp); - // SwitchTargets: convenience method for enabling range-based `for` iteration over a switch block's targets, e.g.: - // for (BasicBlock* const bTarget : block->SwitchTargets()) ... + // SwitchCases: convenience method for enabling range-based `for` iteration over a switch block's cases, e.g.: + // for (BasicBlock* const bTarget : block->SwitchCases()) ... // - BBSwitchTargetList SwitchTargets() const + BBSwitchTargetList SwitchCases() const { assert(bbKind == BBJ_SWITCH); return BBSwitchTargetList(bbSwtTargets); } + // SwitchSuccs: convenience method for enabling range-based `for` iteration over a switch block's unique successors, + // e.g.: + // for (BasicBlock* const bTarget : block->SwitchSuccs()) ... + // + BBJumpTableList SwitchSuccs() const + { + assert(bbKind == BBJ_SWITCH); + return BBJumpTableList((BBJumpTable*)bbSwtTargets); + } + // EHFinallyRetSuccs: convenience method for enabling range-based `for` iteration over BBJ_EHFINALLYRET block // successors, e.g.: // for (BasicBlock* const succ : block->EHFinallyRetSuccs()) ... @@ -2283,6 +2293,70 @@ class BasicBlockRangeList bool ComplexityExceeds(Compiler* comp, unsigned limit, unsigned* count = nullptr); }; +// BBJumpTable -- descriptor blocks with N successors +// +struct BBJumpTable +{ +protected: + FlowEdge** succs; // array of unique `FlowEdge*` pointing to the block's successors + unsigned succCount; // Number of unique successors + +public: + BBJumpTable() + : succs(nullptr) + , succCount(0) + { + } + + BBJumpTable(FlowEdge** succs, unsigned succCount) + : succs(succs) + , succCount(succCount) + { + } + + BBJumpTable(Compiler* comp, const BBJumpTable* other); + + FlowEdge** GetSuccs() const + { + return succs; + } + + FlowEdge* GetSucc(unsigned index) const + { + assert(index < succCount); + assert(succs != nullptr); + return succs[index]; + } + + unsigned GetSuccCount() const + { + return succCount; + } + + void SetSuccs(FlowEdge** newSuccs, unsigned newSuccCount) + { + assert((newSuccs != nullptr) || (newSuccCount == 0)); + + succs = newSuccs; + succCount = newSuccCount; + } + + void RemoveSucc(unsigned index) + { + assert(index < succCount); + assert(succs != nullptr); + + // If succEdge is not the last entry, move everything after in the table down one slot. + if ((index + 1) < succCount) + { + memmove_s(succs + index, (succCount - index) * sizeof(FlowEdge*), succs + index + 1, + (succCount - index - 1) * sizeof(FlowEdge*)); + } + + succCount--; + } +}; + // BBswtDesc -- descriptor for a switch block // // Things to know: @@ -2293,10 +2367,15 @@ class BasicBlockRangeList // switches with just a default case to a BBJ_ALWAYS branch, and a switch with just two cases to a BBJ_COND. // However, in debuggable code, we might not do that, so bbsCount might be 1. // -struct BBswtDesc +struct BBswtDesc : public BBJumpTable { - FlowEdge** bbsDstTab; // case label table address - unsigned bbsCount; // count of cases (includes 'default' if bbsHasDefault) +private: + // Inherited from BBJumpTable: + // FlowEdge** succs; // array of unique `FlowEdge*` pointing to the block's successors + // unsigned succCount; // Number of unique successors + + unsigned caseCount; // count of cases (includes 'default' if bbsHasDefault) + FlowEdge** cases; // array of non-unique FlowEdge* pointing to the switch cases // Case number of most likely case // (only known with PGO, only valid if bbsHasDominantCase is true) @@ -2305,107 +2384,116 @@ struct BBswtDesc bool bbsHasDefault; // true if last switch case is a default case bool bbsHasDominantCase; // true if switch has a dominant case - BBswtDesc() - : bbsHasDefault(true) +public: + BBswtDesc(FlowEdge** succs, unsigned succCount, FlowEdge** cases, unsigned caseCount, bool hasDefault) + : BBJumpTable(succs, succCount) + , caseCount(caseCount) + , cases(cases) + , bbsDominantCase(0) + , bbsHasDefault(hasDefault) , bbsHasDominantCase(false) { } - BBswtDesc(const BBswtDesc* other); + BBswtDesc(FlowEdge** succs, + unsigned succCount, + FlowEdge** cases, + unsigned caseCount, + bool hasDefault, + unsigned dominantCase) + : BBJumpTable(succs, succCount) + , caseCount(caseCount) + , cases(cases) + , bbsDominantCase(dominantCase) + , bbsHasDefault(hasDefault) + , bbsHasDominantCase(true) + { + } BBswtDesc(Compiler* comp, const BBswtDesc* other); - void removeDefault() + FlowEdge** GetCases() const { - assert(bbsHasDefault); - assert(bbsCount > 0); - bbsHasDefault = false; - bbsCount--; + assert((cases != nullptr) || (caseCount == 0)); + return cases; } - FlowEdge* getDefault() + FlowEdge* GetCase(unsigned index) const { - assert(bbsHasDefault); - assert(bbsCount > 0); - return bbsDstTab[bbsCount - 1]; + assert(index < caseCount); + return cases[index]; } -}; -// BBSwitchTargetList out-of-class-declaration implementations (here due to C++ ordering requirements). -// - -inline BBSwitchTargetList::BBSwitchTargetList(BBswtDesc* bbsDesc) - : m_bbsDesc(bbsDesc) -{ - assert(m_bbsDesc != nullptr); - assert(m_bbsDesc->bbsDstTab != nullptr); -} - -inline BBArrayIterator BBSwitchTargetList::begin() const -{ - return BBArrayIterator(m_bbsDesc->bbsDstTab); -} - -inline BBArrayIterator BBSwitchTargetList::end() const -{ - return BBArrayIterator(m_bbsDesc->bbsDstTab + m_bbsDesc->bbsCount); -} - -// BBJumpTable -- descriptor blocks with N successors -// -struct BBJumpTable -{ -private: - FlowEdge** const succs; // array of unique `FlowEdge*` pointing to the block's successors - unsigned succCount; // Number of unique successors - -public: - BBJumpTable() - : succs(nullptr) - , succCount(0) + unsigned GetCaseCount() const { + return caseCount; } - BBJumpTable(FlowEdge** succs, unsigned succCount) - : succs(succs) - , succCount(succCount) + void RemoveDefaultCase() { + assert(bbsHasDefault); + assert(caseCount > 0); + bbsHasDefault = false; + caseCount--; } - BBJumpTable(Compiler* comp, const BBJumpTable* other); - - FlowEdge** GetSuccs() const + bool HasDefaultCase() const { - return succs; + return bbsHasDefault; } - FlowEdge* GetSucc(unsigned index) const + FlowEdge* GetDefaultCase() const { - assert(index < succCount); - return succs[index]; + assert(bbsHasDefault); + assert(caseCount > 0); + return cases[caseCount - 1]; } - unsigned GetSuccCount() const + void SetDominantCase(unsigned dominantCase) { - return succCount; + assert(!bbsHasDominantCase); + bbsDominantCase = dominantCase; + bbsHasDominantCase = true; } - void RemoveSucc(unsigned index) + void RemoveDominantCase() { - assert(index < succCount); - assert(succs != nullptr); + assert(bbsHasDominantCase); + bbsHasDominantCase = false; + } - // If succEdge is not the last entry, move everything after in the table down one slot. - if ((index + 1) < succCount) - { - memmove_s(&succs[index], (succCount - index) * sizeof(FlowEdge*), &succs[index + 1], - (succCount - index - 1) * sizeof(FlowEdge*)); - } + bool HasDominantCase() const + { + return bbsHasDominantCase; + } - succCount--; + unsigned GetDominantCase() const + { + assert(bbsHasDominantCase); + return bbsDominantCase; } }; +// BBSwitchTargetList out-of-class-declaration implementations (here due to C++ ordering requirements). +// + +inline BBSwitchTargetList::BBSwitchTargetList(BBswtDesc* bbsDesc) + : m_bbsDesc(bbsDesc) +{ + assert(m_bbsDesc != nullptr); + assert(m_bbsDesc->GetCases() != nullptr); +} + +inline BBArrayIterator BBSwitchTargetList::begin() const +{ + return BBArrayIterator(m_bbsDesc->GetCases()); +} + +inline BBArrayIterator BBSwitchTargetList::end() const +{ + return BBArrayIterator(m_bbsDesc->GetCases() + m_bbsDesc->GetCaseCount()); +} + // BBJumpTableList out-of-class-declaration implementations (here due to C++ ordering requirements). // @@ -2489,9 +2577,9 @@ inline BasicBlock::SuccList::SuccList(const BasicBlock* block) case BBJ_SWITCH: // We don't use the m_succs in-line data for switches; use the existing jump table in the block. assert(block->bbSwtTargets != nullptr); - assert(block->bbSwtTargets->bbsDstTab != nullptr); - m_begin = block->bbSwtTargets->bbsDstTab; - m_end = block->bbSwtTargets->bbsDstTab + block->bbSwtTargets->bbsCount; + assert(block->bbSwtTargets->GetCases() != nullptr); + m_begin = block->bbSwtTargets->GetCases(); + m_end = block->bbSwtTargets->GetCases() + block->bbSwtTargets->GetCaseCount(); break; default: diff --git a/src/coreclr/jit/clrjit.natvis b/src/coreclr/jit/clrjit.natvis index c87546a448596a..b38738ea9eddb5 100644 --- a/src/coreclr/jit/clrjit.natvis +++ b/src/coreclr/jit/clrjit.natvis @@ -22,7 +22,7 @@ Documentation for VS debugger format specifiers: https://learn.microsoft.com/vis BB{bbNum,d}->BB{bbTargetEdge->m_destBlock->bbNum,d}; {bbKind,en} BB{bbNum,d}-> (BB{bbTrueEdge->m_destBlock->bbNum,d}(T),BB{bbFalseEdge->m_destBlock->bbNum,d}(F)) ; {bbKind,en} - BB{bbNum,d}; {bbKind,en}; {bbSwtTargets->bbsCount} cases + BB{bbNum,d}; {bbKind,en}; {bbSwtTargets->caseCount} cases BB{bbNum,d}; {bbKind,en}; {bbEhfTargets->succCount} succs BB{bbNum,d}; {bbKind,en} diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 22ec04909917e9..3e2ba356790a42 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -485,7 +485,7 @@ void CodeGen::genMarkLabelsForCodegen() break; case BBJ_SWITCH: - for (BasicBlock* const bTarget : block->SwitchTargets()) + for (BasicBlock* const bTarget : block->SwitchSuccs()) { JITDUMP(" " FMT_BB " : switch target\n", bTarget->bbNum); bTarget->SetFlags(BBF_HAS_LABEL); @@ -5564,17 +5564,16 @@ unsigned CodeGen::genEmitJumpTable(GenTree* treeNode, bool relativeAddr) noway_assert(compiler->compCurBB->KindIs(BBJ_SWITCH)); assert(treeNode->OperIs(GT_JMPTABLE)); - emitter* emit = GetEmitter(); - const unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount; - FlowEdge** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab; - const unsigned jmpTabBase = emit->emitBBTableDataGenBeg(jumpCount, relativeAddr); + emitter* emit = GetEmitter(); + const unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->GetCaseCount(); + FlowEdge** const jumpTable = compiler->compCurBB->GetSwitchTargets()->GetCases(); + const unsigned jmpTabBase = emit->emitBBTableDataGenBeg(jumpCount, relativeAddr); JITDUMP("\n J_M%03u_DS%02u LABEL DWORD\n", compiler->compMethodID, jmpTabBase); for (unsigned i = 0; i < jumpCount; i++) { - BasicBlock* target = (*jumpTable)->getDestinationBlock(); - jumpTable++; + BasicBlock* target = jumpTable[i]->getDestinationBlock(); noway_assert(target->HasFlag(BBF_HAS_LABEL)); JITDUMP(" DD L_M%03u_" FMT_BB "\n", compiler->compMethodID, target->bbNum); diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 36e35ba3ecc190..71a5eb894c2d56 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6027,45 +6027,6 @@ class Compiler PhaseStatus fgInsertGCPolls(); BasicBlock* fgCreateGCPoll(GCPollType pollType, BasicBlock* block); -public: - // For many purposes, it is desirable to be able to enumerate the *distinct* targets of a switch statement, - // skipping duplicate targets. (E.g., in flow analyses that are only interested in the set of possible targets.) - // SwitchUniqueSuccSet contains the non-duplicated switch successor edges. - // Code that modifies the flowgraph (such as by renumbering blocks) must call Compiler::InvalidateUniqueSwitchSuccMap, - // and code that modifies the targets of a switch block must call Compiler::fgInvalidateSwitchDescMapEntry. - // If the unique targets of a switch block are needed later, they will be recomputed, ensuring they're up-to-date. - struct SwitchUniqueSuccSet - { - unsigned numDistinctSuccs; // Number of distinct targets of the switch. - FlowEdge** nonDuplicates; // Array of "numDistinctSuccs", containing all the distinct switch target - // successor edges. - }; - - typedef JitHashTable, SwitchUniqueSuccSet> BlockToSwitchDescMap; - -private: - // Maps BasicBlock*'s that end in switch statements to SwitchUniqueSuccSets that allow - // iteration over only the distinct successors. - BlockToSwitchDescMap* m_switchDescMap = nullptr; - -public: - BlockToSwitchDescMap* GetSwitchDescMap(bool createIfNull = true) - { - if ((m_switchDescMap == nullptr) && createIfNull) - { - m_switchDescMap = new (getAllocator()) BlockToSwitchDescMap(getAllocator()); - } - return m_switchDescMap; - } - - SwitchUniqueSuccSet GetDescriptorForSwitch(BasicBlock* switchBlk); - - bool GetDescriptorForSwitchIfAvailable(BasicBlock* switchBlk, SwitchUniqueSuccSet* res); - - void fgRemoveSuccFromSwitchDescMapEntry(BasicBlock* switchBlk, FlowEdge* edge); - - void fgInvalidateSwitchDescMapEntry(BasicBlock* switchBlk); - BasicBlock* fgFirstBlockOfHandler(BasicBlock* block); FlowEdge* fgGetPredForBlock(BasicBlock* block, BasicBlock* blockPred); diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 3560d0e1632fa0..eb10348a70b6b0 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -711,10 +711,9 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func, const bool case BBJ_SWITCH: { - Compiler::SwitchUniqueSuccSet sd = comp->GetDescriptorForSwitch(this); - for (unsigned i = 0; i < sd.numDistinctSuccs; i++) + for (unsigned i = 0; i < bbSwtTargets->GetSuccCount(); i++) { - RETURN_ON_ABORT(func(sd.nonDuplicates[i]->getDestinationBlock())); + RETURN_ON_ABORT(func(bbSwtTargets->GetSucc(i)->getDestinationBlock())); } return VisitEHSuccs(comp, func); @@ -778,10 +777,9 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func) case BBJ_SWITCH: { - Compiler::SwitchUniqueSuccSet sd = comp->GetDescriptorForSwitch(this); - for (unsigned i = 0; i < sd.numDistinctSuccs; i++) + for (unsigned i = 0; i < bbSwtTargets->GetSuccCount(); i++) { - RETURN_ON_ABORT(func(sd.nonDuplicates[i]->getDestinationBlock())); + RETURN_ON_ABORT(func(bbSwtTargets->GetSucc(i)->getDestinationBlock())); } return BasicBlockVisit::Continue; diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 9815fccb9c04ba..31db7a111715c1 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -204,42 +204,15 @@ void Compiler::fgChangeSwitchBlock(BasicBlock* oldSwitchBlock, BasicBlock* newSw // Walk the switch's jump table, updating the predecessor for each branch. BBswtDesc* swtDesc = oldSwitchBlock->GetSwitchTargets(); - for (unsigned i = 0; i < swtDesc->bbsCount; i++) + for (unsigned i = 0; i < swtDesc->GetSuccCount(); i++) { - FlowEdge* succEdge = swtDesc->bbsDstTab[i]; - assert(succEdge != nullptr); + FlowEdge* succEdge = swtDesc->GetSucc(i); + assert(succEdge->getSourceBlock() == oldSwitchBlock); - if (succEdge->getSourceBlock() != oldSwitchBlock) - { - // swtDesc can have duplicate targets, so we may have updated this edge already - // - assert(succEdge->getSourceBlock() == newSwitchBlock); - assert(succEdge->getDupCount() > 1); - } - else - { - // Redirect edge's source block from oldSwitchBlock to newSwitchBlock, - // and keep successor block's pred list in order - // - fgReplacePred(succEdge, newSwitchBlock); - } - } - - if (m_switchDescMap != nullptr) - { - SwitchUniqueSuccSet uniqueSuccSet; - - // If already computed and cached the unique descriptors for the old block, let's - // update those for the new block. - if (m_switchDescMap->Lookup(oldSwitchBlock, &uniqueSuccSet)) - { - m_switchDescMap->Set(newSwitchBlock, uniqueSuccSet, BlockToSwitchDescMap::Overwrite); - } - else - { - fgInvalidateSwitchDescMapEntry(newSwitchBlock); - } - fgInvalidateSwitchDescMapEntry(oldSwitchBlock); + // Redirect edge's source block from oldSwitchBlock to newSwitchBlock, + // and keep successor block's pred list in order + // + fgReplacePred(succEdge, newSwitchBlock); } } @@ -468,8 +441,8 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas case BBJ_SWITCH: { - unsigned const jumpCnt = block->GetSwitchTargets()->bbsCount; - FlowEdge** const jumpTab = block->GetSwitchTargets()->bbsDstTab; + unsigned const jumpCnt = block->GetSwitchTargets()->GetCaseCount(); + FlowEdge** const jumpTab = block->GetSwitchTargets()->GetCases(); FlowEdge* oldEdge = nullptr; FlowEdge* newEdge = nullptr; bool changed = false; @@ -520,8 +493,15 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas assert(newEdge->getDestinationBlock() == newTarget); newEdge->addLikelihood(oldEdge->getLikelihood()); - // Remove 'oldEdge' from the switch map entry, if it exists. - fgRemoveSuccFromSwitchDescMapEntry(block, oldEdge); + for (unsigned i = block->GetSwitchTargets()->GetSuccCount(); i != 0; i--) + { + if (block->GetSwitchTargets()->GetSucc(i - 1) == oldEdge) + { + // Remove the old edge from the unique successor table. + block->GetSwitchTargets()->RemoveSucc(i - 1); + break; + } + } } // If we simply redirected 'oldEdge' to 'newTarget', we don't need to update the switch map entry, @@ -2835,26 +2815,34 @@ void Compiler::fgLinkBasicBlocks() case BBJ_SWITCH: { - const unsigned numSucc = curBBdesc->GetSwitchTargets()->bbsCount; - unsigned jumpCnt = numSucc; - FlowEdge** jumpPtr = curBBdesc->GetSwitchTargets()->bbsDstTab; + const unsigned numCases = curBBdesc->GetSwitchTargets()->GetCaseCount(); + FlowEdge** const cases = curBBdesc->GetSwitchTargets()->GetCases(); + FlowEdge** const succs = cases - numCases; + unsigned numUnique = 0; - do + for (unsigned i = 0; i < numCases; i++) { - BasicBlock* jumpDest = fgLookupBB((unsigned)*(size_t*)jumpPtr); + BasicBlock* jumpDest = fgLookupBB((unsigned)*(size_t*)(cases + i)); FlowEdge* const newEdge = fgAddRefPred(jumpDest, curBBdesc); - newEdge->setLikelihood((1.0 / numSucc) * newEdge->getDupCount()); - *jumpPtr = newEdge; - if (jumpDest->bbNum <= curBBdesc->bbNum) + newEdge->setLikelihood((1.0 / numCases) * newEdge->getDupCount()); + cases[i] = newEdge; + + if (newEdge->getDupCount() == 1) { - fgMarkBackwardJump(jumpDest, curBBdesc); + succs[numUnique++] = newEdge; + if (jumpDest->bbNum <= curBBdesc->bbNum) + { + fgMarkBackwardJump(jumpDest, curBBdesc); + } } - } while (++jumpPtr, --jumpCnt); + } + + curBBdesc->GetSwitchTargets()->SetSuccs(succs, numUnique); - /* Default case of CEE_SWITCH (next block), is at end of jumpTab[] */ + /* Default case of CEE_SWITCH (next block), is at end of cases[] */ - noway_assert(curBBdesc->NextIs((*(jumpPtr - 1))->getDestinationBlock())); + noway_assert(curBBdesc->NextIs(cases[numCases - 1]->getDestinationBlock())); break; } @@ -3013,56 +3001,42 @@ void Compiler::fgMakeBasicBlocks(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed case CEE_SWITCH: { - unsigned jmpBase; - unsigned jmpCnt; // # of switch cases (excluding default) - - FlowEdge** jmpTab; - FlowEdge** jmpPtr; - - /* Allocate the switch descriptor */ - - swtDsc = new (this, CMK_BasicBlock) BBswtDesc; - /* Read the number of entries in the table */ - jmpCnt = getU4LittleEndian(codeAddr); + const unsigned jmpCnt = getU4LittleEndian(codeAddr); // # of switch cases (excluding default) codeAddr += 4; /* Compute the base offset for the opcode */ - jmpBase = (IL_OFFSET)((codeAddr - codeBegp) + jmpCnt * sizeof(DWORD)); + const unsigned jmpBase = (IL_OFFSET)((codeAddr - codeBegp) + jmpCnt * sizeof(DWORD)); - /* Allocate the jump table */ + /* Allocate the jump table, ensuring there's space for all cases, the default case, and unique succs */ - jmpPtr = jmpTab = new (this, CMK_FlowEdge) FlowEdge*[jmpCnt + 1]; + FlowEdge** const jmpTab = new (this, CMK_FlowEdge) FlowEdge*[(jmpCnt + 1) * 2]; + FlowEdge** const cases = jmpTab + (jmpCnt + 1); /* Fill in the jump table */ - for (unsigned count = jmpCnt; count; count--) + for (unsigned i = 0; i < jmpCnt; i++) { jmpDist = getI4LittleEndian(codeAddr); codeAddr += 4; // store the offset in the pointer. We change these in fgLinkBasicBlocks(). - *jmpPtr++ = (FlowEdge*)(size_t)(jmpBase + jmpDist); + cases[i] = (FlowEdge*)(size_t)(jmpBase + jmpDist); } /* Append the default label to the target table */ - *jmpPtr++ = (FlowEdge*)(size_t)jmpBase; - - /* Make sure we found the right number of labels */ - - noway_assert(jmpPtr == jmpTab + jmpCnt + 1); + cases[jmpCnt] = (FlowEdge*)(size_t)jmpBase; /* Compute the size of the switch opcode operands */ sz = sizeof(DWORD) + jmpCnt * sizeof(DWORD); - /* Fill in the remaining fields of the switch descriptor */ + /* Allocate the switch descriptor; we will initialize the unique successors in fgLinkBasicBlocks */ - swtDsc->bbsCount = jmpCnt + 1; - swtDsc->bbsDstTab = jmpTab; + swtDsc = new (this, CMK_BasicBlock) BBswtDesc(nullptr, 0, cases, jmpCnt + 1, true); /* This is definitely a jump */ @@ -4197,7 +4171,7 @@ void Compiler::fgCheckBasicBlockControlFlow() break; case BBJ_SWITCH: // block ends with a switch statement - for (BasicBlock* const bTarget : blk->SwitchTargets()) + for (BasicBlock* const bTarget : blk->SwitchCases()) { fgControlFlowPermitted(blk, bTarget); } diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index fbd28bec04e4cd..ceceb3d5d0f4e5 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -1068,7 +1068,7 @@ bool Compiler::fgDumpFlowGraph(Phases phase, PhasePosition pos) { fprintf(fgxFile, "\n switchCases=\"%d\"", edge->getDupCount()); } - if (bSource->GetSwitchTargets()->getDefault()->getDestinationBlock() == bTarget) + if (bSource->GetSwitchTargets()->GetDefaultCase()->getDestinationBlock() == bTarget) { fprintf(fgxFile, "\n switchDefault=\"true\""); } @@ -2037,22 +2037,22 @@ void Compiler::fgTableDispBasicBlock(const BasicBlock* block, printedBlockWidth = 2 + 9 /* kind */; const BBswtDesc* const jumpSwt = block->GetSwitchTargets(); - const unsigned jumpCnt = jumpSwt->bbsCount; - FlowEdge** const jumpTab = jumpSwt->bbsDstTab; + const unsigned jumpCnt = jumpSwt->GetCaseCount(); + FlowEdge** const jumpTab = jumpSwt->GetCases(); for (unsigned i = 0; i < jumpCnt; i++) { printedBlockWidth += 1 /* space/comma */; printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i])); - const bool isDefault = jumpSwt->bbsHasDefault && (i == jumpCnt - 1); + const bool isDefault = jumpSwt->HasDefaultCase() && (i == jumpCnt - 1); if (isDefault) { printf("[def]"); printedBlockWidth += 5; } - const bool isDominant = jumpSwt->bbsHasDominantCase && (i == jumpSwt->bbsDominantCase); + const bool isDominant = jumpSwt->HasDominantCase() && (i == jumpSwt->GetDominantCase()); if (isDominant) { printf("[dom]"); @@ -2783,7 +2783,7 @@ bool BBPredsChecker::CheckJump(BasicBlock* blockPred, BasicBlock* block) break; case BBJ_SWITCH: - for (BasicBlock* const bTarget : blockPred->SwitchTargets()) + for (BasicBlock* const bTarget : blockPred->SwitchSuccs()) { if (block == bTarget) { @@ -2967,38 +2967,10 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef maxBBNum = max(maxBBNum, block->bbNum); - // 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. - if (block->KindIs(BBJ_SWITCH)) - { - for (BasicBlock* const succBlock : block->Succs()) - { - assert(succBlock->bbTraversalStamp == curTraversalStamp); - } - - // Also check the unique successor set, if it exists. Make sure to NOT allocate it if it doesn't exist! - BlockToSwitchDescMap* switchMap = GetSwitchDescMap(/* createIfNull */ false); - if (switchMap != nullptr) - { - SwitchUniqueSuccSet sd; - if (switchMap->Lookup(block, &sd)) - { - for (unsigned i = 0; i < sd.numDistinctSuccs; i++) - { - const BasicBlock* const nonDuplicateSucc = sd.nonDuplicates[i]->getDestinationBlock(); - assert(nonDuplicateSucc != nullptr); - assert(nonDuplicateSucc->bbTraversalStamp == curTraversalStamp); - } - } - } - } - else + // Check that all the successors have the current traversal stamp. + for (BasicBlock* const succBlock : block->Succs(this)) { - for (BasicBlock* const succBlock : block->Succs(this)) - { - assert(succBlock->bbTraversalStamp == curTraversalStamp); - } + assert(succBlock->bbTraversalStamp == curTraversalStamp); } // If the block is a BBJ_COND, a BBJ_SWITCH or a @@ -3968,31 +3940,23 @@ void Compiler::fgDebugCheckBlockLinks() } // If this is a switch, check that the tables are consistent. - // Note that we don't call GetSwitchDescMap(), because it has the side-effect - // of allocating it if it is not present. - if (block->KindIs(BBJ_SWITCH) && m_switchDescMap != nullptr) + if (block->KindIs(BBJ_SWITCH)) { - SwitchUniqueSuccSet uniqueSuccSet; - if (m_switchDescMap->Lookup(block, &uniqueSuccSet)) + // Create a set with all the successors. + BitVecTraits bitVecTraits(fgBBNumMax + 1, this); + BitVec succBlocks(BitVecOps::MakeEmpty(&bitVecTraits)); + for (BasicBlock* const bTarget : block->SwitchCases()) { - // Create a set with all the successors. Don't use BlockSet, so we don't need to worry - // about the BlockSet epoch. - BitVecTraits bitVecTraits(fgBBNumMax + 1, this); - BitVec succBlocks(BitVecOps::MakeEmpty(&bitVecTraits)); - for (BasicBlock* const bTarget : block->SwitchTargets()) - { - BitVecOps::AddElemD(&bitVecTraits, succBlocks, bTarget->bbNum); - } - // Now we should have a set of unique successors that matches what's in the switchMap. - // First, check the number of entries, then make sure all the blocks in uniqueSuccSet - // are in the BlockSet. - unsigned count = BitVecOps::Count(&bitVecTraits, succBlocks); - assert(uniqueSuccSet.numDistinctSuccs == count); - for (unsigned i = 0; i < uniqueSuccSet.numDistinctSuccs; i++) - { - assert(BitVecOps::IsMember(&bitVecTraits, succBlocks, - uniqueSuccSet.nonDuplicates[i]->getDestinationBlock()->bbNum)); - } + BitVecOps::AddElemD(&bitVecTraits, succBlocks, bTarget->bbNum); + } + // Now we should have a set of unique successors that matches what's in the switchMap. + // First, check the number of entries, then make sure all the blocks in the unique successor table + // match the blocks in the set. + unsigned count = BitVecOps::Count(&bitVecTraits, succBlocks); + assert(block->GetSwitchTargets()->GetSuccCount() == count); + for (BasicBlock* const bTarget : block->SwitchSuccs()) + { + assert(BitVecOps::IsMember(&bitVecTraits, succBlocks, bTarget->bbNum)); } } } diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 240252a4558a4c..c22bde11ea5955 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -323,9 +323,9 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block) case BBJ_SWITCH: { BBswtDesc* const swtDesc = block->GetSwitchTargets(); - for (unsigned i = 0; i < swtDesc->bbsCount; i++) + for (unsigned i = 0; i < swtDesc->GetSuccCount(); i++) { - fgRemoveRefPred(swtDesc->bbsDstTab[i]); + fgRemoveAllRefPreds(swtDesc->GetSucc(i)->getDestinationBlock(), block); } break; } @@ -404,142 +404,3 @@ void Compiler::fgRedirectEdge(FlowEdge*& edge, BasicBlock* newTarget) // Pred list of target should still be ordered assert(newTarget->checkPredListOrder()); } - -//------------------------------------------------------------------------ -// GetDescriptorForSwitch: Returns the SwitchUniqueSuccSet corresponding to 'switchBlk'. -// If it does not exist in the map yet, we build and insert the entry. -// -// Arguments: -// switchBlk -- The switch block -// -// Returns: -// The SwitchUniqueSuccSet corresponding to 'switchBlk' -// -Compiler::SwitchUniqueSuccSet Compiler::GetDescriptorForSwitch(BasicBlock* switchBlk) -{ - assert(switchBlk->KindIs(BBJ_SWITCH)); - BlockToSwitchDescMap* switchMap = GetSwitchDescMap(); - SwitchUniqueSuccSet res; - if (switchMap->Lookup(switchBlk, &res)) - { - return res; - } - else - { - // We must compute the descriptor. Find which are dups, by creating a bit set with the unique successors. - // We create a temporary bitset of blocks to compute the unique set of successor blocks, - // since adding a block's number twice leaves just one "copy" in the bitset. - - BitVecTraits blockVecTraits(fgBBNumMax + 1, this); - BitVec uniqueSuccBlocks(BitVecOps::MakeEmpty(&blockVecTraits)); - for (BasicBlock* const targ : switchBlk->SwitchTargets()) - { - BitVecOps::AddElemD(&blockVecTraits, uniqueSuccBlocks, targ->bbNum); - } - // Now we have a set of unique successors. - unsigned numNonDups = BitVecOps::Count(&blockVecTraits, uniqueSuccBlocks); - - FlowEdge** nonDups = new (getAllocator()) FlowEdge*[numNonDups]; - - unsigned nonDupInd = 0; - - // At this point, all unique targets are in "uniqueSuccBlocks". As we encounter each, - // add to nonDups, remove from "uniqueSuccBlocks". - BBswtDesc* const swtDesc = switchBlk->GetSwitchTargets(); - for (unsigned i = 0; i < swtDesc->bbsCount; i++) - { - FlowEdge* const succEdge = swtDesc->bbsDstTab[i]; - BasicBlock* const targ = succEdge->getDestinationBlock(); - if (BitVecOps::IsMember(&blockVecTraits, uniqueSuccBlocks, targ->bbNum)) - { - nonDups[nonDupInd] = succEdge; - nonDupInd++; - BitVecOps::RemoveElemD(&blockVecTraits, uniqueSuccBlocks, targ->bbNum); - } - } - - assert(nonDupInd == numNonDups); - assert(BitVecOps::Count(&blockVecTraits, uniqueSuccBlocks) == 0); - res.numDistinctSuccs = numNonDups; - res.nonDuplicates = nonDups; - switchMap->Set(switchBlk, res); - return res; - } -} - -//------------------------------------------------------------------------ -// GetDescriptorForSwitchIfAvailable: Gets the SwitchUniqueSuccSet corresponding to 'switchBlk', -// if it exists. Unlike Compiler::GetDescriptorForSwitch, this will not modify the map. -// -// Arguments: -// switchBlk -- The switch block -// res [out] -- Pointer to the SwitchUniqueSuccSet to populate -// -// Returns: -// True if the map exists, and contains an entry for 'switchBlk' -// -bool Compiler::GetDescriptorForSwitchIfAvailable(BasicBlock* switchBlk, SwitchUniqueSuccSet* res) -{ - assert(switchBlk->KindIs(BBJ_SWITCH)); - return (m_switchDescMap != nullptr) && m_switchDescMap->Lookup(switchBlk, res); -} - -//------------------------------------------------------------------------ -// fgRemoveSuccFromSwitchDescMapEntry: Removes a successor edge from the map entry -// for 'switchBlk', if the entry exists. -// -// Arguments: -// switchBlk -- The switch block -// edge -- The successor edge to remove -// -void Compiler::fgRemoveSuccFromSwitchDescMapEntry(BasicBlock* switchBlk, FlowEdge* edge) -{ - assert(switchBlk->KindIs(BBJ_SWITCH)); - - SwitchUniqueSuccSet uniqueSuccSet; - if (!GetDescriptorForSwitchIfAvailable(switchBlk, &uniqueSuccSet)) - { - return; - } - - const unsigned succCount = uniqueSuccSet.numDistinctSuccs; - FlowEdge** const succTab = uniqueSuccSet.nonDuplicates; - bool found = false; - assert(succCount > 0); - assert(succTab != nullptr); - - for (unsigned i = 0; !found && (i < succCount); i++) - { - if (succTab[i] == edge) - { - // If 'edge' is not the last entry, move everything after in the table down one slot. - if ((i + 1) < succCount) - { - memmove_s(&succTab[i], (succCount - i) * sizeof(FlowEdge*), &succTab[i + 1], - (succCount - i - 1) * sizeof(FlowEdge*)); - } - - found = true; - } - } - - assert(found); - uniqueSuccSet.numDistinctSuccs--; - m_switchDescMap->Set(switchBlk, uniqueSuccSet, BlockToSwitchDescMap::SetKind::Overwrite); -} - -//------------------------------------------------------------------------ -// fgInvalidateSwitchDescMapEntry: Removes the entry for 'block' from the -// switch map, if the map exists. -// -// Arguments: -// block -- The switch block -// -void Compiler::fgInvalidateSwitchDescMapEntry(BasicBlock* block) -{ - // Check if map has no entries yet. - if (m_switchDescMap != nullptr) - { - m_switchDescMap->Remove(block); - } -} diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 9b63fb63cfd5ee..c8e2689cd2f550 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1588,17 +1588,13 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block) { assert(block->KindIs(BBJ_SWITCH)); - unsigned jmpCnt = block->GetSwitchTargets()->bbsCount; - FlowEdge** jmpTab = block->GetSwitchTargets()->bbsDstTab; - BasicBlock* bNewDest; // the new jump target for the current switch case - BasicBlock* bDest; - bool modified = false; + bool modified = false; - do + for (unsigned i = 0; i < block->GetSwitchTargets()->GetSuccCount(); i++) { - REPEAT_SWITCH:; - bDest = (*jmpTab)->getDestinationBlock(); - bNewDest = bDest; + FlowEdge* const edge = block->GetSwitchTargets()->GetSucc(i); + BasicBlock* const bDest = edge->getDestinationBlock(); + BasicBlock* bNewDest = bDest; // 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 @@ -1616,14 +1612,9 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block) if (optimizeJump) { bNewDest = bDest->GetTarget(); -#ifdef DEBUG - if (verbose) - { - printf("\nOptimizing a switch jump to an empty block with an unconditional jump (" FMT_BB - " -> " FMT_BB " -> " FMT_BB ")\n", - block->bbNum, bDest->bbNum, bNewDest->bbNum); - } -#endif // DEBUG + JITDUMP("\nOptimizing a switch jump to an empty block with an unconditional jump (" FMT_BB " -> " FMT_BB + " -> " FMT_BB ")\n", + block->bbNum, bDest->bbNum, bNewDest->bbNum); } } @@ -1633,8 +1624,7 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block) // if (bDest->hasProfileWeight()) { - FlowEdge* const oldEdge = *jmpTab; - bDest->decreaseBBProfileWeight(oldEdge->getLikelyWeight()); + bDest->decreaseBBProfileWeight(edge->getLikelyWeight()); } // Redirect the jump to the new target @@ -1642,10 +1632,11 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block) fgReplaceJumpTarget(block, bDest, bNewDest); modified = true; - // we optimized a Switch label - goto REPEAT_SWITCH to follow this new jump - goto REPEAT_SWITCH; + // Try optimizing this edge again + // + i--; } - } while (++jmpTab, --jmpCnt); + } if (modified) { @@ -1678,24 +1669,15 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block) // At this point all of the case jump targets have been updated such // that none of them go to block that is an empty unconditional block - // - jmpTab = block->GetSwitchTargets()->bbsDstTab; - jmpCnt = block->GetSwitchTargets()->bbsCount; - // Now check for two trivial switch jumps. // - if (block->NumSucc(this) == 1) + if (block->GetSwitchTargets()->GetSuccCount() == 1) { // Use BBJ_ALWAYS for a switch with only a default clause, or with only one unique successor. -#ifdef DEBUG - if (verbose) - { - printf("\nRemoving a switch jump with a single target (" FMT_BB ")\n", block->bbNum); - printf("BEFORE:\n"); - fgDispBasicBlocks(); - } -#endif // DEBUG + JITDUMP("\nRemoving a switch jump with a single target (" FMT_BB ")\n", block->bbNum); + JITDUMP("BEFORE:\n"); + DBEXEC(verbose, fgDispBasicBlocks()); if (block->IsLIR()) { @@ -1765,15 +1747,13 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block) } // Change the switch jump into a BBJ_ALWAYS - block->SetKindAndTargetEdge(BBJ_ALWAYS, block->GetSwitchTargets()->bbsDstTab[0]); - for (unsigned i = 1; i < jmpCnt; ++i) - { - fgRemoveRefPred(jmpTab[i]); - } - + block->SetKindAndTargetEdge(BBJ_ALWAYS, block->GetSwitchTargets()->GetCase(0)); + const unsigned dupCount = block->GetTargetEdge()->getDupCount(); + block->GetTargetEdge()->decrementDupCount(dupCount - 1); + block->GetTarget()->bbRefs -= (dupCount - 1); return true; } - else if (block->GetSwitchTargets()->bbsCount == 2) + else if (block->GetSwitchTargets()->GetCaseCount() == 2) { /* Use a BBJ_COND(switchVal==0) for a switch with only one significant clause besides the default clause */ @@ -1795,16 +1775,10 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block) // a COMMA node which results in noway asserts in fgMorphSmpOp(), optAssertionGen() and rpPredictTreeRegUse(). // For the same reason fgMorphSmpOp() marks GT_JTRUE nodes with RELOP children as GTF_DONT_CSE. -#ifdef DEBUG - if (verbose) - { - printf("\nConverting a switch (" FMT_BB ") with only one significant clause besides a default target to a " - "conditional branch. Before:\n", - block->bbNum); - - gtDispTree(switchTree); - } -#endif // DEBUG + JITDUMP("\nConverting a switch (" FMT_BB ") with only one significant clause besides a default target to a " + "conditional branch. Before:\n", + block->bbNum); + DISPNODE(switchTree); switchTree->ChangeOper(GT_JTRUE); GenTree* zeroConstNode = gtNewZeroConNode(genActualType(switchVal->TypeGet())); @@ -1824,8 +1798,8 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block) fgSetStmtSeq(switchStmt); } - FlowEdge* const trueEdge = block->GetSwitchTargets()->bbsDstTab[0]; - FlowEdge* const falseEdge = block->GetSwitchTargets()->bbsDstTab[1]; + FlowEdge* const trueEdge = block->GetSwitchTargets()->GetCase(0); + FlowEdge* const falseEdge = block->GetSwitchTargets()->GetCase(1); block->SetCond(trueEdge, falseEdge); JITDUMP("After:\n"); @@ -2778,7 +2752,7 @@ bool Compiler::fgOptimizeBranch(BasicBlock* bJump) void Compiler::fgPeelSwitch(BasicBlock* block) { assert(block->KindIs(BBJ_SWITCH)); - assert(block->GetSwitchTargets()->bbsHasDominantCase); + assert(block->GetSwitchTargets()->HasDominantCase()); assert(!block->isRunRarely()); // Lowering expands switches, so calling this method on lowered IR @@ -2790,13 +2764,13 @@ void Compiler::fgPeelSwitch(BasicBlock* block) // assert(block->hasProfileWeight()); - const unsigned dominantCase = block->GetSwitchTargets()->bbsDominantCase; + const unsigned dominantCase = block->GetSwitchTargets()->GetDominantCase(); JITDUMP(FMT_BB " has switch with dominant case %u, considering peeling\n", block->bbNum, dominantCase); // The dominant case should not be the default case, as we already peel that one. // - assert(dominantCase < (block->GetSwitchTargets()->bbsCount - 1)); - FlowEdge* const dominantEdge = block->GetSwitchTargets()->bbsDstTab[dominantCase]; + assert(dominantCase < (block->GetSwitchTargets()->GetCaseCount() - 1)); + FlowEdge* const dominantEdge = block->GetSwitchTargets()->GetCase(dominantCase); BasicBlock* const dominantTarget = dominantEdge->getDestinationBlock(); Statement* const switchStmt = block->lastStmt(); GenTree* const switchTree = switchStmt->GetRootNode(); @@ -2856,9 +2830,8 @@ void Compiler::fgPeelSwitch(BasicBlock* block) // and increase all other case likelihoods proportionally. // dominantEdge->setLikelihood(BB_ZERO_WEIGHT); - const SwitchUniqueSuccSet uniqueSuccSet = GetDescriptorForSwitch(newBlock); - const unsigned numSucc = uniqueSuccSet.numDistinctSuccs; - FlowEdge** const jumpTab = uniqueSuccSet.nonDuplicates; + const unsigned numSucc = newBlock->GetSwitchTargets()->GetSuccCount(); + FlowEdge** const jumpTab = newBlock->GetSwitchTargets()->GetSuccs(); for (unsigned i = 0; i < numSucc; i++) { // If we removed all of the flow out of 'block', distribute flow among the remaining edges evenly. @@ -2872,7 +2845,7 @@ void Compiler::fgPeelSwitch(BasicBlock* block) // // But it no longer has a dominant case. // - newBlock->GetSwitchTargets()->bbsHasDominantCase = false; + newBlock->GetSwitchTargets()->RemoveDominantCase(); if (fgNodeThreading == NodeThreading::AllTrees) { diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index d3a43675162298..f98a9aba0a8896 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -4136,8 +4136,8 @@ void EfficientEdgeCountReconstructor::MarkInterestingSwitches(BasicBlock* block, // If it turns out often we fail at this stage, we might consider building a histogram of switch case // values at runtime, similar to what we do for classes at virtual call sites. // - const unsigned caseCount = block->GetSwitchTargets()->bbsCount; - FlowEdge** const jumpTab = block->GetSwitchTargets()->bbsDstTab; + const unsigned caseCount = block->GetSwitchTargets()->GetCaseCount(); + FlowEdge** const jumpTab = block->GetSwitchTargets()->GetCases(); unsigned dominantCase = caseCount; for (unsigned i = 0; i < caseCount; i++) @@ -4164,7 +4164,7 @@ void EfficientEdgeCountReconstructor::MarkInterestingSwitches(BasicBlock* block, return; } - if (block->GetSwitchTargets()->bbsHasDefault && (dominantCase == caseCount - 1)) + if (block->GetSwitchTargets()->HasDefaultCase() && (dominantCase == caseCount - 1)) { // Dominant case is the default case. // This effectively gets peeled already, so defer. @@ -4178,8 +4178,7 @@ void EfficientEdgeCountReconstructor::MarkInterestingSwitches(BasicBlock* block, "; marking for peeling\n", dominantCase, dominantEdge->m_targetBlock->bbNum, fraction); - block->GetSwitchTargets()->bbsHasDominantCase = true; - block->GetSwitchTargets()->bbsDominantCase = dominantCase; + block->GetSwitchTargets()->SetDominantCase(dominantCase); } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index bb012edec025f5..69c534cdb0d31b 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -8095,8 +8095,8 @@ void Compiler::impImportBlockCode(BasicBlock* block) { // Find the jump target size_t switchVal = (size_t)op1->AsIntCon()->gtIconVal; - unsigned jumpCnt = block->GetSwitchTargets()->bbsCount; - FlowEdge** jumpTab = block->GetSwitchTargets()->bbsDstTab; + unsigned jumpCnt = block->GetSwitchTargets()->GetCaseCount(); + FlowEdge** jumpTab = block->GetSwitchTargets()->GetCases(); bool foundVal = false; Metrics.ImporterSwitchFold++; @@ -8123,15 +8123,11 @@ void Compiler::impImportBlockCode(BasicBlock* block) } assert(foundVal); -#ifdef DEBUG - if (verbose) - { - printf("\nSwitch folded at " FMT_BB "\n", block->bbNum); - printf(FMT_BB " becomes a %s", block->bbNum, "BBJ_ALWAYS"); - printf(" to " FMT_BB, block->GetTarget()->bbNum); - printf("\n"); - } -#endif + JITDUMP("\nSwitch folded at " FMT_BB "\n", block->bbNum); + JITDUMP(FMT_BB " becomes a %s", block->bbNum, "BBJ_ALWAYS"); + JITDUMP(" to " FMT_BB, block->GetTarget()->bbNum); + JITDUMP("\n"); + if (block->hasProfileWeight()) { // We are unlikely to be able to repair the profile. @@ -11876,7 +11872,7 @@ void Compiler::impImportBlock(BasicBlock* block) addStmt = impExtractLastStmt(); assert(addStmt->GetRootNode()->OperIs(GT_SWITCH)); - for (BasicBlock* const tgtBlock : block->SwitchTargets()) + for (BasicBlock* const tgtBlock : block->SwitchSuccs()) { multRef |= tgtBlock->bbRefs; diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 9395f1c83889a5..d03edd32d21831 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -944,9 +944,11 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // jumpCnt is the number of elements in the jump table array. // jumpTab is the actual pointer to the jump table array. // targetCnt is the number of unique targets in the jump table array. - const unsigned jumpCnt = originalSwitchBB->GetSwitchTargets()->bbsCount; - FlowEdge** const jumpTab = originalSwitchBB->GetSwitchTargets()->bbsDstTab; - const unsigned targetCnt = originalSwitchBB->NumSucc(comp); + // uniqueSuccs is the array of the switch's unique successors. + const unsigned jumpCnt = originalSwitchBB->GetSwitchTargets()->GetCaseCount(); + FlowEdge** const jumpTab = originalSwitchBB->GetSwitchTargets()->GetCases(); + unsigned targetCnt = originalSwitchBB->GetSwitchTargets()->GetSuccCount(); + FlowEdge** const uniqueSuccs = originalSwitchBB->GetSwitchTargets()->GetSuccs(); // GT_SWITCH must be a top-level node with no use. #ifdef DEBUG @@ -968,10 +970,9 @@ GenTree* Lowering::LowerSwitch(GenTree* node) originalSwitchBB->SetKindAndTargetEdge(BBJ_ALWAYS, jumpTab[0]); // Remove extra predecessor links if there was more than one case. - for (unsigned i = 1; i < jumpCnt; ++i) - { - comp->fgRemoveRefPred(jumpTab[i]); - } + const unsigned dupCount = originalSwitchBB->GetTargetEdge()->getDupCount(); + originalSwitchBB->GetTargetEdge()->decrementDupCount(dupCount - 1); + originalSwitchBB->GetTarget()->bbRefs -= (dupCount - 1); // We have to get rid of the GT_SWITCH node but a child might have side effects so just assign // the result of the child subtree to a temp. @@ -1053,7 +1054,7 @@ GenTree* Lowering::LowerSwitch(GenTree* node) assert(originalSwitchBB->TargetIs(afterDefaultCondBlock)); assert(originalSwitchBB->JumpsToNext()); assert(afterDefaultCondBlock->KindIs(BBJ_SWITCH)); - assert(afterDefaultCondBlock->GetSwitchTargets()->bbsHasDefault); + assert(afterDefaultCondBlock->GetSwitchTargets()->HasDefaultCase()); assert(afterDefaultCondBlock->isEmpty()); // Nothing here yet. // The GT_SWITCH code is still in originalSwitchBB (it will be removed later). @@ -1097,36 +1098,16 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // If we originally had 2 unique successors, check to see whether there is a unique // non-default case, in which case we can eliminate the switch altogether. // Note that the single unique successor case is handled above. - FlowEdge* uniqueSucc = nullptr; - if (targetCnt == 2) - { - uniqueSucc = jumpTab[0]; - noway_assert(jumpCnt >= 2); - for (unsigned i = 1; i < jumpCnt - 1; i++) - { - if (jumpTab[i] != uniqueSucc) - { - uniqueSucc = nullptr; - break; - } - } - } - if (uniqueSucc != nullptr) + if ((targetCnt == 2) && (defaultEdge->getDupCount() == 0)) { - // If the unique successor immediately follows this block, we have nothing to do - - // it will simply fall-through after we remove the switch, below. - // Otherwise, make this a BBJ_ALWAYS. - // Now, fixup the predecessor links to uniqueSucc's target block. In the original jumpTab: - // jumpTab[i-1] was the default target, which we handled above, - // jumpTab[0] is the first target, and we'll leave that predecessor link. - // Remove any additional predecessor links to uniqueSucc's target block. - for (unsigned i = 1; i < jumpCnt - 1; ++i) - { - assert(jumpTab[i] == uniqueSucc); - comp->fgRemoveRefPred(uniqueSucc); - } - + // The default case was peeled off, and we have only one other unique successor. + // Jump directly to this remaining successor. + FlowEdge* const uniqueSucc = (uniqueSuccs[0] == defaultEdge) ? uniqueSuccs[1] : uniqueSuccs[0]; + assert(uniqueSucc != defaultEdge); afterDefaultCondBlock->SetKindAndTargetEdge(BBJ_ALWAYS, uniqueSucc); + const unsigned dupCount = uniqueSucc->getDupCount(); + uniqueSucc->decrementDupCount(dupCount - 1); + uniqueSucc->getDestinationBlock()->bbRefs -= (dupCount - 1); } // If the number of possible destinations is small enough, we proceed to expand the switch // into a series of conditional branches, otherwise we follow the jump table based switch @@ -1309,9 +1290,9 @@ GenTree* Lowering::LowerSwitch(GenTree* node) if (afterDefaultCondBlock->hasProfileWeight()) { bool profileInconsistent = false; - for (unsigned i = 0; i < jumpCnt - 1; i++) + for (unsigned i = 0; i < targetCnt; i++) { - BasicBlock* const targetBlock = jumpTab[i]->getDestinationBlock(); + BasicBlock* const targetBlock = uniqueSuccs[i]->getDestinationBlock(); targetBlock->setBBProfileWeight(targetBlock->computeIncomingWeight()); profileInconsistent |= (targetBlock->NumSucc() > 0); } @@ -1355,24 +1336,29 @@ GenTree* Lowering::LowerSwitch(GenTree* node) switchBlockRange.InsertAfter(switchValue, switchTable, switchJump); // This block no longer has a default switch case. - // If no other cases branch to this successor, remove it from the switch map entry. + // If no other cases branch to this successor, remove it from the unique successor table. if (defaultEdge->getDupCount() == 0) { - comp->fgRemoveSuccFromSwitchDescMapEntry(afterDefaultCondBlock, defaultEdge); + for (unsigned i = 0; i < targetCnt; i++) + { + FlowEdge* const edge = uniqueSuccs[i]; + if (edge == defaultEdge) + { + afterDefaultCondBlock->GetSwitchTargets()->RemoveSucc(i); + break; + } + } + + assert(targetCnt == (afterDefaultCondBlock->GetSwitchTargets()->GetSuccCount() + 1)); + targetCnt--; } - afterDefaultCondBlock->GetSwitchTargets()->removeDefault(); + afterDefaultCondBlock->GetSwitchTargets()->RemoveDefaultCase(); // We need to scale up the likelihood of the remaining switch edges, now that we've peeled off // the default case. But if the remaining likelihood is zero (default likelihood was 1.0), // we don't know the case likelihoods. Instead, divide likelihood evenly among all cases. // - // First, rebuild the unique succ set - // - Compiler::SwitchUniqueSuccSet successors = comp->GetDescriptorForSwitch(afterDefaultCondBlock); - - // Then fix each successor edge - // if (Compiler::fgProfileWeightsEqual(defaultLikelihood, 1.0, 0.001)) { JITDUMP("Zero weight switch block " FMT_BB ", distributing likelihoods equally per case\n", @@ -1380,9 +1366,9 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // jumpCnt-1 here because we peeled the default after copying this value. weight_t const newLikelihood = 1.0 / (jumpCnt - 1); bool profileInconsistent = false; - for (unsigned i = 0; i < successors.numDistinctSuccs; i++) + for (unsigned i = 0; i < targetCnt; i++) { - FlowEdge* const edge = successors.nonDuplicates[i]; + FlowEdge* const edge = uniqueSuccs[i]; weight_t const oldEdgeWeight = edge->getLikelyWeight(); edge->setLikelihood(newLikelihood * edge->getDupCount()); weight_t const newEdgeWeight = edge->getLikelyWeight(); @@ -1407,9 +1393,9 @@ GenTree* Lowering::LowerSwitch(GenTree* node) weight_t const scaleFactor = 1.0 / (1.0 - defaultLikelihood); JITDUMP("Scaling switch block " FMT_BB " likelihoods by " FMT_WT "\n", afterDefaultCondBlock->bbNum, scaleFactor); - for (unsigned i = 0; i < successors.numDistinctSuccs; i++) + for (unsigned i = 0; i < targetCnt; i++) { - FlowEdge* const edge = successors.nonDuplicates[i]; + FlowEdge* const edge = uniqueSuccs[i]; weight_t newLikelihood = scaleFactor * edge->getLikelihood(); if (newLikelihood > 1.0) @@ -1422,11 +1408,6 @@ GenTree* Lowering::LowerSwitch(GenTree* node) } } } - else - { - // 'afterDefaultCondBlock' is no longer a switch block. Remove its switch map entry. - comp->fgInvalidateSwitchDescMapEntry(afterDefaultCondBlock); - } } GenTree* next = node->gtNext; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 81e465ce46c1e0..2612d9b45daf49 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12818,8 +12818,8 @@ Compiler::FoldResult Compiler::fgFoldConditional(BasicBlock* block) // Find the actual jump target size_t switchVal = (size_t)cond->AsIntCon()->gtIconVal; - unsigned jumpCnt = block->GetSwitchTargets()->bbsCount; - FlowEdge** jumpTab = block->GetSwitchTargets()->bbsDstTab; + unsigned jumpCnt = block->GetSwitchTargets()->GetCaseCount(); + FlowEdge** jumpTab = block->GetSwitchTargets()->GetCases(); bool foundVal = false; bool profileInconsistent = false; diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index a2bd4dd7d7c833..3cdea868da54dc 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -615,12 +615,12 @@ void Compiler::optSetMappedBlockTargets(BasicBlock* blk, BasicBlock* newBlk, Blo case BBJ_SWITCH: { BBswtDesc* currSwtDesc = blk->GetSwitchTargets(); - BBswtDesc* newSwtDesc = new (this, CMK_BasicBlock) BBswtDesc(currSwtDesc); - newSwtDesc->bbsDstTab = new (this, CMK_FlowEdge) FlowEdge*[newSwtDesc->bbsCount]; + BBswtDesc* newSwtDesc = new (this, CMK_BasicBlock) BBswtDesc(this, currSwtDesc); + FlowEdge** succPtr = newSwtDesc->GetSuccs(); - for (unsigned i = 0; i < newSwtDesc->bbsCount; i++) + for (unsigned i = 0; i < newSwtDesc->GetCaseCount(); i++) { - FlowEdge* const inspiringEdge = currSwtDesc->bbsDstTab[i]; + FlowEdge* const inspiringEdge = currSwtDesc->GetCase(i); BasicBlock* const switchTarget = inspiringEdge->getDestinationBlock(); FlowEdge* newEdge; @@ -636,13 +636,16 @@ void Compiler::optSetMappedBlockTargets(BasicBlock* blk, BasicBlock* newBlk, Blo // Transfer likelihood... instead of doing this gradually // for dup'd edges, we set it once, when we add the last dup. + // Also, add the new edge to the unique successor table. // if (newEdge->getDupCount() == inspiringEdge->getDupCount()) { newEdge->setLikelihood(inspiringEdge->getLikelihood()); + *succPtr = newEdge; + succPtr++; } - newSwtDesc->bbsDstTab[i] = newEdge; + newSwtDesc->GetCases()[i] = newEdge; } newBlk->SetSwitch(newSwtDesc); diff --git a/src/coreclr/jit/switchrecognition.cpp b/src/coreclr/jit/switchrecognition.cpp index 6818a6b15b8987..163171cc69b76e 100644 --- a/src/coreclr/jit/switchrecognition.cpp +++ b/src/coreclr/jit/switchrecognition.cpp @@ -42,12 +42,12 @@ PhaseStatus Compiler::optRecognizeAndOptimizeSwitchJumps() modified = true; // Converted switches won't have dominant cases, so we can skip the switch peeling check. - assert(!block->GetSwitchTargets()->bbsHasDominantCase); + assert(!block->GetSwitchTargets()->HasDominantCase()); } else #endif - if (block->KindIs(BBJ_SWITCH) && block->GetSwitchTargets()->bbsHasDominantCase) + if (block->KindIs(BBJ_SWITCH) && block->GetSwitchTargets()->HasDominantCase()) { fgPeelSwitch(block); modified = true; @@ -377,7 +377,12 @@ bool Compiler::optSwitchConvert( FlowEdge* const falseEdge = firstBlock->GetFalseEdge(); // Convert firstBlock to a switch block - firstBlock->SetSwitch(new (this, CMK_BasicBlock) BBswtDesc); + const unsigned jumpCount = static_cast(maxValue - minValue + 1); + assert((jumpCount > 0) && (jumpCount <= SWITCH_MAX_DISTANCE + 1)); + FlowEdge** const jmpTab = + new (this, CMK_FlowEdge) FlowEdge*[2 + jumpCount + 1 /* true/false edges | cases | default case */]; + + firstBlock->SetSwitch(new (this, CMK_BasicBlock) BBswtDesc(jmpTab, 2, jmpTab + 2, jumpCount + 1, true)); firstBlock->bbCodeOffsEnd = lastBlock->bbCodeOffsEnd; firstBlock->lastStmt()->GetRootNode()->ChangeOper(GT_SWITCH); @@ -406,14 +411,7 @@ bool Compiler::optSwitchConvert( blockToRemove = nextBlockToRemove; } - const unsigned jumpCount = static_cast(maxValue - minValue + 1); - assert((jumpCount > 0) && (jumpCount <= SWITCH_MAX_DISTANCE + 1)); - FlowEdge** jmpTab = new (this, CMK_FlowEdge) FlowEdge*[jumpCount + 1 /*default case*/]; - - fgHasSwitch = true; - firstBlock->GetSwitchTargets()->bbsCount = jumpCount + 1; - firstBlock->GetSwitchTargets()->bbsHasDefault = true; - firstBlock->GetSwitchTargets()->bbsDstTab = jmpTab; + fgHasSwitch = true; // Splitting doesn't work well with jump-tables currently opts.compProcedureSplitting = false; @@ -430,7 +428,8 @@ bool Compiler::optSwitchConvert( // Unlink blockIfTrue from firstBlock, we're going to link it again in the loop below. fgRemoveRefPred(trueEdge); - FlowEdge* switchTrueEdge = nullptr; + FlowEdge* switchTrueEdge = nullptr; + FlowEdge** const cases = jmpTab + 2; for (unsigned i = 0; i < jumpCount; i++) { @@ -438,7 +437,7 @@ bool Compiler::optSwitchConvert( const bool isTrue = (bitVector & static_cast(1ULL << i)) != 0; FlowEdge* const newEdge = fgAddRefPred((isTrue ? blockIfTrue : blockIfFalse), firstBlock); - jmpTab[i] = newEdge; + cases[i] = newEdge; if ((switchTrueEdge == nullptr) && isTrue) { @@ -450,11 +449,15 @@ bool Compiler::optSwitchConvert( // Link the 'default' case FlowEdge* const switchDefaultEdge = fgAddRefPred(blockIfFalse, firstBlock); - jmpTab[jumpCount] = switchDefaultEdge; + cases[jumpCount] = switchDefaultEdge; // Fix likelihoods switchDefaultEdge->setLikelihood(falseLikelihood); switchTrueEdge->setLikelihood(1.0 - falseLikelihood); + // Initialize unique successor table + firstBlock->GetSwitchTargets()->GetSuccs()[0] = cases[0]; + firstBlock->GetSwitchTargets()->GetSuccs()[1] = (cases[0] == switchTrueEdge) ? switchDefaultEdge : switchTrueEdge; + return true; } From 4cddaf0ff234d14d1a6142061e255e8d4ac2cf53 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 15 Jul 2025 18:05:53 -0400 Subject: [PATCH 3/5] SwitchCases -> SwitchSuccs --- src/coreclr/jit/fgbasic.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 31db7a111715c1..1eea36f2a9ae86 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -4171,7 +4171,7 @@ void Compiler::fgCheckBasicBlockControlFlow() break; case BBJ_SWITCH: // block ends with a switch statement - for (BasicBlock* const bTarget : blk->SwitchCases()) + for (BasicBlock* const bTarget : blk->SwitchSuccs()) { fgControlFlowPermitted(blk, bTarget); } From 01733218c75442fcd7ba8f548cb5a11f3517d5ce Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 15 Jul 2025 19:27:08 -0400 Subject: [PATCH 4/5] Remove SwitchCases iterator --- src/coreclr/jit/block.h | 45 +------------------------------- src/coreclr/jit/fgdiagnostic.cpp | 3 ++- 2 files changed, 3 insertions(+), 45 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 66f6cc319f71bb..e66eb5e726d2b1 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -382,22 +382,8 @@ class FlowEdgeArrayIterator } }; -// BBSwitchTargetList: adapter class for forward iteration of switch targets, using range-based `for`, -// normally used via BasicBlock::SwitchCases(), e.g.: -// for (BasicBlock* const target : block->SwitchCases()) ... -// -class BBSwitchTargetList -{ - BBswtDesc* m_bbsDesc; - -public: - BBSwitchTargetList(BBswtDesc* bbsDesc); - BBArrayIterator begin() const; - BBArrayIterator end() const; -}; - // BBJumpTableList: adapter class for forward iteration of blocks with N successors, using range-based `for`, -// normally used via BasicBlock::EHFinallyRetSuccs(), e.g.: +// normally used via BasicBlock::EHFinallyRetSuccs() or BasicBlock::SwitchSuccs(), e.g.: // for (BasicBlock* const succ : block->EHFinallyRetSuccs()) ... // class BBJumpTableList @@ -1408,15 +1394,6 @@ struct BasicBlock : private LIR::Range BasicBlock* GetSucc(unsigned i) const; BasicBlock* GetSucc(unsigned i, Compiler* comp); - // SwitchCases: convenience method for enabling range-based `for` iteration over a switch block's cases, e.g.: - // for (BasicBlock* const bTarget : block->SwitchCases()) ... - // - BBSwitchTargetList SwitchCases() const - { - assert(bbKind == BBJ_SWITCH); - return BBSwitchTargetList(bbSwtTargets); - } - // SwitchSuccs: convenience method for enabling range-based `for` iteration over a switch block's unique successors, // e.g.: // for (BasicBlock* const bTarget : block->SwitchSuccs()) ... @@ -2474,26 +2451,6 @@ struct BBswtDesc : public BBJumpTable } }; -// BBSwitchTargetList out-of-class-declaration implementations (here due to C++ ordering requirements). -// - -inline BBSwitchTargetList::BBSwitchTargetList(BBswtDesc* bbsDesc) - : m_bbsDesc(bbsDesc) -{ - assert(m_bbsDesc != nullptr); - assert(m_bbsDesc->GetCases() != nullptr); -} - -inline BBArrayIterator BBSwitchTargetList::begin() const -{ - return BBArrayIterator(m_bbsDesc->GetCases()); -} - -inline BBArrayIterator BBSwitchTargetList::end() const -{ - return BBArrayIterator(m_bbsDesc->GetCases() + m_bbsDesc->GetCaseCount()); -} - // BBJumpTableList out-of-class-declaration implementations (here due to C++ ordering requirements). // diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index ceceb3d5d0f4e5..9cbc76ceb772e1 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3945,8 +3945,9 @@ void Compiler::fgDebugCheckBlockLinks() // Create a set with all the successors. BitVecTraits bitVecTraits(fgBBNumMax + 1, this); BitVec succBlocks(BitVecOps::MakeEmpty(&bitVecTraits)); - for (BasicBlock* const bTarget : block->SwitchCases()) + for (unsigned i = 0; i < block->GetSwitchTargets()->GetCaseCount(); i++) { + BasicBlock* const bTarget = block->GetSwitchTargets()->GetCase(i)->getDestinationBlock(); BitVecOps::AddElemD(&bitVecTraits, succBlocks, bTarget->bbNum); } // Now we should have a set of unique successors that matches what's in the switchMap. From 126d4b3356e02b1892b8577971ecdc195f7e153a Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 15 Jul 2025 19:29:45 -0400 Subject: [PATCH 5/5] Add comment --- src/coreclr/jit/block.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index e66eb5e726d2b1..65d1e312d7d953 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -2343,6 +2343,10 @@ struct BBJumpTable // allows for a degenerate switch with zero cases. Normally, the optimizer will optimize degenerate // switches with just a default case to a BBJ_ALWAYS branch, and a switch with just two cases to a BBJ_COND. // However, in debuggable code, we might not do that, so bbsCount might be 1. +// 3. BBswtDesc makes no promises about the relative positions of the 'succs' and 'cases' arrays. +// Callers are responsible for allocating these arrays during BBswtDesc creation. +// A potential optimization is to allocate one array large enough for the two; +// this is safe, because BBswtDesc does not support adding new cases/successors. // struct BBswtDesc : public BBJumpTable {