From 09cfa9cb8e7cfc7002bf92e13349e95cd565a99c Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 14 Feb 2024 15:30:00 -0500 Subject: [PATCH 1/6] Update BBehfDesc to use flow edges --- src/coreclr/jit/block.h | 4 +-- src/coreclr/jit/fgbasic.cpp | 43 ++++++++++++++++---------------- src/coreclr/jit/fgdiagnostic.cpp | 4 +-- src/coreclr/jit/importer.cpp | 4 +-- 4 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 041af02b1c15ad..35c950057a9923 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -1879,8 +1879,8 @@ inline BBArrayIterator BBSwitchTargetList::end() const // struct BBehfDesc { - BasicBlock** bbeSuccs; // array of `BasicBlock*` pointing to BBJ_EHFINALLYRET block successors - unsigned bbeCount; // size of `bbeSuccs` array + FlowEdge** bbeSuccs; // array of `FlowEdge*` pointing to BBJ_EHFINALLYRET block successors + unsigned bbeCount; // size of `bbeSuccs` array BBehfDesc() : bbeSuccs(nullptr), bbeCount(0) { diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 9ba1d20cd4bd6b..9d8d8e42470fe4 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -449,18 +449,17 @@ void Compiler::fgChangeEhfBlock(BasicBlock* oldBlock, BasicBlock* newBlock) assert(oldBlock->KindIs(BBJ_EHFINALLYRET)); assert(fgPredsComputed); - for (BasicBlock* const succ : oldBlock->EHFinallyRetSuccs()) - { - assert(succ != nullptr); + BBehfDesc* ehfDesc = oldBlock->GetEhfTargets(); - // Remove the old edge [oldBlock => succ] - // - assert(succ->countOfInEdges() > 0); - fgRemoveRefPred(succ, oldBlock); + for (unsigned i = 0; i < ehfDesc->bbeCount; i++) + { + FlowEdge* succEdge = ehfDesc->bbeSuccs[i]; + assert(succEdge != nullptr); - // Create the new edge [newBlock => succ] + // Redirect edge's source block from oldBlock to newBlock // - fgAddRefPred(succ, newBlock); + assert(succEdge->getSourceBlock() == oldBlock); + succEdge->setSourceBlock(newBlock); } } @@ -486,19 +485,19 @@ void Compiler::fgReplaceEhfSuccessor(BasicBlock* block, BasicBlock* oldSucc, Bas BBehfDesc* const ehfDesc = block->GetEhfTargets(); const unsigned succCount = ehfDesc->bbeCount; - BasicBlock** const succTab = ehfDesc->bbeSuccs; + FlowEdge** const succTab = ehfDesc->bbeSuccs; // Walk the successor table looking for the old successor, which we expect to find. unsigned oldSuccNum = UINT_MAX; unsigned newSuccNum = UINT_MAX; for (unsigned i = 0; i < succCount; i++) { - if (succTab[i] == newSucc) + if (succTab[i]->getDestinationBlock() == newSucc) { newSuccNum = i; } - if (succTab[i] == oldSucc) + if (succTab[i]->getSourceBlock() == oldSucc) { oldSuccNum = i; } @@ -517,17 +516,17 @@ void Compiler::fgReplaceEhfSuccessor(BasicBlock* block, BasicBlock* oldSucc, Bas } else { - // Replace the old one with the new one. - - succTab[oldSuccNum] = newSucc; - // Remove the old edge [block => oldSucc] // fgRemoveAllRefPreds(oldSucc, block); // Create the new edge [block => newSucc] // - fgAddRefPred(newSucc, block); + FlowEdge* const newEdge = fgAddRefPred(newSucc, block); + + // Replace the old one with the new one. + // + succTab[oldSuccNum] = newEdge; JITDUMP("Replace BBJ_EHFINALLYRET " FMT_BB " successor " FMT_BB " with " FMT_BB "\n", block->bbNum, oldSucc->bbNum, newSucc->bbNum); @@ -556,19 +555,19 @@ void Compiler::fgRemoveEhfSuccessor(BasicBlock* block, BasicBlock* succ) BBehfDesc* const ehfDesc = block->GetEhfTargets(); unsigned succCount = ehfDesc->bbeCount; - BasicBlock** succTab = ehfDesc->bbeSuccs; + FlowEdge** succTab = ehfDesc->bbeSuccs; bool found = false; // Walk the successor table looking for the specified successor block. for (unsigned i = 0; i < succCount; i++) { - if (succTab[i] == succ) + if (succTab[i]->getDestinationBlock() == succ) { // If it's not the last one, move everything after in the table down one slot. if (i + 1 < succCount) { - memmove_s(&succTab[i], (succCount - i) * sizeof(BasicBlock*), &succTab[i + 1], - (succCount - i - 1) * sizeof(BasicBlock*)); + memmove_s(&succTab[i], (succCount - i) * sizeof(FlowEdge*), &succTab[i + 1], + (succCount - i - 1) * sizeof(FlowEdge*)); } --succCount; @@ -579,7 +578,7 @@ void Compiler::fgRemoveEhfSuccessor(BasicBlock* block, BasicBlock* succ) // We only expect to see a successor once in the table. for (; i < succCount; i++) { - assert(succTab[i] != succ); + assert(succTab[i]->getDestinationBlock() != succ); } #endif // DEBUG diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 65248487fb118e..ff0397b69f0b12 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -2014,12 +2014,12 @@ 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; - BasicBlock** const jumpTab = ehfDesc->bbeSuccs; + FlowEdge** const jumpTab = ehfDesc->bbeSuccs; for (unsigned i = 0; i < jumpCnt; i++) { printedBlockWidth += 1 /* space/comma */; - printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i])); + printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i]->getDestinationBlock())); } } diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index b3d035b26d96bc..196054a044a50b 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -12274,7 +12274,7 @@ void Compiler::impFixPredLists() if (predCount > 0) { jumpEhf->bbeCount = predCount; - jumpEhf->bbeSuccs = new (this, CMK_BasicBlock) BasicBlock*[predCount]; + jumpEhf->bbeSuccs = new (this, CMK_FlowEdge) FlowEdge*[predCount]; unsigned predNum = 0; for (BasicBlock* const predBlock : finallyBegBlock->PredBlocks()) @@ -12290,7 +12290,7 @@ void Compiler::impFixPredLists() FlowEdge* const newEdge = fgAddRefPred(continuation, finallyBlock); newEdge->setLikelihood(1.0 / predCount); - jumpEhf->bbeSuccs[predNum] = continuation; + jumpEhf->bbeSuccs[predNum] = newEdge; ++predNum; if (!added) From b7a02d4c0d2d144d22af84f8493d6a7d31736c9e Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 15 Feb 2024 17:34:45 -0500 Subject: [PATCH 2/6] Use FlowEdge pointers for BBJ_EHFINALLYRET successors --- src/coreclr/jit/block.cpp | 12 +++--- src/coreclr/jit/block.h | 74 +++++++++++++++++++++++++------- src/coreclr/jit/compiler.hpp | 4 +- src/coreclr/jit/fgbasic.cpp | 6 +-- src/coreclr/jit/fgdiagnostic.cpp | 4 +- src/coreclr/jit/optimizer.cpp | 12 +++--- 6 files changed, 78 insertions(+), 34 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 01ead6622e6e14..58b91b739fe424 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -658,12 +658,12 @@ void BasicBlock::dspKind() const } else { - const unsigned jumpCnt = bbEhfTargets->bbeCount; - BasicBlock** const jumpTab = bbEhfTargets->bbeSuccs; + const unsigned jumpCnt = bbEhfTargets->bbeCount; + FlowEdge** const jumpTab = bbEhfTargets->bbeSuccs; for (unsigned i = 0; i < jumpCnt; i++) { - printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i])); + printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i]->getDestinationBlock())); } } @@ -1214,7 +1214,7 @@ BasicBlock* BasicBlock::GetSucc(unsigned i) const } case BBJ_EHFINALLYRET: - return bbEhfTargets->bbeSuccs[i]; + return bbEhfTargets->bbeSuccs[i]->getDestinationBlock(); case BBJ_SWITCH: return bbSwtTargets->bbsDstTab[i]; @@ -1315,7 +1315,7 @@ BasicBlock* BasicBlock::GetSucc(unsigned i, Compiler* comp) case BBJ_EHFINALLYRET: assert(bbEhfTargets != nullptr); assert(i < bbEhfTargets->bbeCount); - return bbEhfTargets->bbeSuccs[i]; + return bbEhfTargets->bbeSuccs[i]->getDestinationBlock(); case BBJ_CALLFINALLY: case BBJ_CALLFINALLYRET: @@ -1792,7 +1792,7 @@ BBehfDesc::BBehfDesc(Compiler* comp, const BBehfDesc* other) : bbeCount(other->b { // Allocate and fill in a new dst tab // - bbeSuccs = new (comp, CMK_BasicBlock) BasicBlock*[bbeCount]; + bbeSuccs = new (comp, CMK_FlowEdge) FlowEdge*[bbeCount]; for (unsigned i = 0; i < bbeCount; i++) { bbeSuccs[i] = other->bbeSuccs[i]; diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 35c950057a9923..09fc1c71c35638 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -314,21 +314,27 @@ class PredBlockList // class BBArrayIterator { - BasicBlock* const* m_bbEntry; + // Quirk: Some BasicBlock kinds refer to their successors with BasicBlock pointers, + // while others use FlowEdge pointers. Eventually, every type will use FlowEdge pointers. + // For now, support iterating with both types. + union { + BasicBlock* const* m_bbEntry; + FlowEdge* const* m_edgeEntry; + }; + + bool iterateEdges; public: - BBArrayIterator(BasicBlock* const* bbEntry) : m_bbEntry(bbEntry) + BBArrayIterator(BasicBlock* const* bbEntry) : m_bbEntry(bbEntry), iterateEdges(false) { } - BasicBlock* operator*() const + BBArrayIterator(FlowEdge* const* edgeEntry) : m_edgeEntry(edgeEntry), iterateEdges(true) { - assert(m_bbEntry != nullptr); - BasicBlock* bTarget = *m_bbEntry; - assert(bTarget != nullptr); - return bTarget; } + BasicBlock* operator*() const; + BBArrayIterator& operator++() { assert(m_bbEntry != nullptr); @@ -1570,9 +1576,22 @@ struct BasicBlock : private LIR::Range // need to call a function or execute another `switch` to get them. Also, pre-compute the begin and end // points of the iteration, for use by BBArrayIterator. `m_begin` and `m_end` will either point at // `m_succs` or at the switch table successor array. - BasicBlock* m_succs[2]; - BasicBlock* const* m_begin; - BasicBlock* const* m_end; + BasicBlock* m_succs[2]; + + // Quirk: Some BasicBlock kinds refer to their successors with BasicBlock pointers, + // while others use FlowEdge pointers. Eventually, every type will use FlowEdge pointers. + // For now, support iterating with both types. + union { + BasicBlock* const* m_begin; + FlowEdge* const* m_beginEdge; + }; + + union { + BasicBlock* const* m_end; + FlowEdge* const* m_endEdge; + }; + + bool iterateEdges; public: BBSuccList(const BasicBlock* block); @@ -1913,6 +1932,8 @@ inline BBArrayIterator BBEhfSuccList::end() const inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block) { assert(block != nullptr); + iterateEdges = false; + switch (block->bbKind) { case BBJ_THROW: @@ -1957,14 +1978,16 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block) // been computed. if (block->GetEhfTargets() == nullptr) { - m_begin = nullptr; - m_end = nullptr; + m_beginEdge = nullptr; + m_endEdge = nullptr; } else { - m_begin = block->GetEhfTargets()->bbeSuccs; - m_end = block->GetEhfTargets()->bbeSuccs + block->GetEhfTargets()->bbeCount; + m_beginEdge = block->GetEhfTargets()->bbeSuccs; + m_endEdge = block->GetEhfTargets()->bbeSuccs + block->GetEhfTargets()->bbeCount; } + + iterateEdges = true; break; case BBJ_SWITCH: @@ -1984,12 +2007,12 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block) inline BBArrayIterator BasicBlock::BBSuccList::begin() const { - return BBArrayIterator(m_begin); + return (iterateEdges ? BBArrayIterator(m_beginEdge) : BBArrayIterator(m_begin)); } inline BBArrayIterator BasicBlock::BBSuccList::end() const { - return BBArrayIterator(m_end); + return (iterateEdges ? BBArrayIterator(m_endEdge) : BBArrayIterator(m_end)); } // We have a simpler struct, BasicBlockList, which is simply a singly-linked @@ -2192,6 +2215,25 @@ struct FlowEdge } }; +// BasicBlock iterator implementations (that are required to be defined after the declaration of FlowEdge) + +inline BasicBlock* BBArrayIterator::operator*() const +{ + if (iterateEdges) + { + assert(m_edgeEntry != nullptr); + FlowEdge* edgeTarget = *m_edgeEntry; + assert(edgeTarget != nullptr); + assert(edgeTarget->getDestinationBlock() != nullptr); + return edgeTarget->getDestinationBlock(); + } + + assert(m_bbEntry != nullptr); + BasicBlock* bTarget = *m_bbEntry; + assert(bTarget != nullptr); + return bTarget; +} + // Pred list iterator implementations (that are required to be defined after the declaration of BasicBlock and FlowEdge) inline PredEdgeList::iterator::iterator(FlowEdge* pred) : m_pred(pred) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 59e53baeb19f8a..67f5c59d93265e 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -657,7 +657,7 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) { for (unsigned i = 0; i < bbEhfTargets->bbeCount; i++) { - RETURN_ON_ABORT(func(bbEhfTargets->bbeSuccs[i])); + RETURN_ON_ABORT(func(bbEhfTargets->bbeSuccs[i]->getDestinationBlock())); } } @@ -732,7 +732,7 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func) { for (unsigned i = 0; i < bbEhfTargets->bbeCount; i++) { - RETURN_ON_ABORT(func(bbEhfTargets->bbeSuccs[i])); + RETURN_ON_ABORT(func(bbEhfTargets->bbeSuccs[i]->getDestinationBlock())); } } diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 9d8d8e42470fe4..eb1457191308ff 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -483,9 +483,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; + BBehfDesc* const ehfDesc = block->GetEhfTargets(); + const unsigned succCount = ehfDesc->bbeCount; + FlowEdge** const succTab = ehfDesc->bbeSuccs; // Walk the successor table looking for the old successor, which we expect to find. unsigned oldSuccNum = UINT_MAX; diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index ff0397b69f0b12..900a58fb404033 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -2013,8 +2013,8 @@ 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; + const unsigned jumpCnt = ehfDesc->bbeCount; + FlowEdge** const jumpTab = ehfDesc->bbeSuccs; for (unsigned i = 0; i < jumpCnt; i++) { diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index c1362b987aab73..f68a6afe02f057 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -643,22 +643,24 @@ void Compiler::optRedirectBlock(BasicBlock* blk, BasicBlock* newBlk, BlockToBloc BBehfDesc* currEhfDesc = blk->GetEhfTargets(); BBehfDesc* newEhfDesc = new (this, CMK_BasicBlock) BBehfDesc; newEhfDesc->bbeCount = currEhfDesc->bbeCount; - newEhfDesc->bbeSuccs = new (this, CMK_BasicBlock) BasicBlock*[newEhfDesc->bbeCount]; - BasicBlock** jumpPtr = newEhfDesc->bbeSuccs; + newEhfDesc->bbeSuccs = new (this, CMK_FlowEdge) FlowEdge*[newEhfDesc->bbeCount]; + FlowEdge** jumpPtr = newEhfDesc->bbeSuccs; for (BasicBlock* const ehfTarget : blk->EHFinallyRetSuccs()) { + FlowEdge* newEdge; + // Determine if newBlk should target ehfTarget, or be redirected if (redirectMap->Lookup(ehfTarget, &newTarget)) { - *jumpPtr = newTarget; + newEdge = fgAddRefPred(newTarget, newBlk); } else { - *jumpPtr = ehfTarget; + newEdge = fgAddRefPred(ehfTarget, newBlk); } - fgAddRefPred(*jumpPtr, newBlk); + *jumpPtr = newEdge; jumpPtr++; } From 364410cec33d888a8c54c3df1286d2fe44cfbf5e Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 15 Feb 2024 21:29:24 -0500 Subject: [PATCH 3/6] Update some methods to use easy edge access --- src/coreclr/jit/compiler.h | 8 ++- src/coreclr/jit/fgbasic.cpp | 138 ++++++++++++++++++++++++++---------- src/coreclr/jit/fgflow.cpp | 45 +++++++++++- 3 files changed, 152 insertions(+), 39 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a18fee959436b4..c850b8f76176a2 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5883,6 +5883,8 @@ class Compiler FlowEdge* fgRemoveRefPred(BasicBlock* block, BasicBlock* blockPred); + void fgRemoveRefPred(FlowEdge* edge); + FlowEdge* fgRemoveAllRefPreds(BasicBlock* block, BasicBlock* blockPred); void fgRemoveBlockAsPred(BasicBlock* block); @@ -5893,12 +5895,16 @@ class Compiler void fgReplaceEhfSuccessor(BasicBlock* block, BasicBlock* oldSucc, BasicBlock* newSucc); - void fgRemoveEhfSuccessor(BasicBlock* block, BasicBlock* succ); + void fgRemoveEhfSuccessor(BasicBlock* block, const unsigned succIndex); + + void fgRemoveEhfSuccessor(FlowEdge* succEdge); void fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, BasicBlock* newTarget); void fgReplacePred(BasicBlock* block, BasicBlock* oldPred, BasicBlock* newPred); + void fgReplacePred(FlowEdge* edge, BasicBlock* const newPred); + // initializingPreds is only 'true' when we are computing preds in fgLinkBasicBlocks() template FlowEdge* fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowEdge* oldEdge = nullptr); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index eb1457191308ff..05ba42dedc2d3d 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -454,12 +454,7 @@ void Compiler::fgChangeEhfBlock(BasicBlock* oldBlock, BasicBlock* newBlock) for (unsigned i = 0; i < ehfDesc->bbeCount; i++) { FlowEdge* succEdge = ehfDesc->bbeSuccs[i]; - assert(succEdge != nullptr); - - // Redirect edge's source block from oldBlock to newBlock - // - assert(succEdge->getSourceBlock() == oldBlock); - succEdge->setSourceBlock(newBlock); + fgReplacePred(succEdge, newBlock); } } @@ -487,18 +482,20 @@ void Compiler::fgReplaceEhfSuccessor(BasicBlock* block, BasicBlock* oldSucc, Bas const unsigned succCount = ehfDesc->bbeCount; FlowEdge** const succTab = ehfDesc->bbeSuccs; - // Walk the successor table looking for the old successor, which we expect to find. + // Walk the successor table looking for the old successor, which we expect to find only once. unsigned oldSuccNum = UINT_MAX; unsigned newSuccNum = UINT_MAX; for (unsigned i = 0; i < succCount; i++) { if (succTab[i]->getDestinationBlock() == newSucc) { + assert(newSuccNum == UINT_MAX); newSuccNum = i; } if (succTab[i]->getSourceBlock() == oldSucc) { + assert(oldSuccNum == UINT_MAX); oldSuccNum = i; } } @@ -508,7 +505,7 @@ void Compiler::fgReplaceEhfSuccessor(BasicBlock* block, BasicBlock* oldSucc, Bas if (newSuccNum != UINT_MAX) { // The new successor is already in the table; simply remove the old one. - fgRemoveEhfSuccessor(block, oldSucc); + fgRemoveEhfSuccessor(block, oldSuccNum); JITDUMP("Remove existing BBJ_EHFINALLYRET " FMT_BB " successor " FMT_BB "; replacement successor " FMT_BB " already exists in list\n", @@ -534,60 +531,96 @@ void Compiler::fgReplaceEhfSuccessor(BasicBlock* block, BasicBlock* oldSucc, Bas } //------------------------------------------------------------------------ -// fgRemoveEhfSuccessor: update BBJ_EHFINALLYRET block to remove `succ` as a successor. -// Updates the predecessor list of `succ`. +// fgRemoveEhfSuccessor: update BBJ_EHFINALLYRET block to remove the successor at `succIndex` +// in the block's jump table. +// Updates the predecessor list of the successor, if necessary. // // Arguments: -// block - BBJ_EHFINALLYRET block -// succ - successor +// block - BBJ_EHFINALLYRET block +// succIndex - index of the successor in block->GetEhfTargets()->bbeSuccs // -void Compiler::fgRemoveEhfSuccessor(BasicBlock* block, BasicBlock* succ) +void Compiler::fgRemoveEhfSuccessor(BasicBlock* block, const unsigned succIndex) { assert(block != nullptr); - assert(succ != nullptr); - assert(fgPredsComputed); assert(block->KindIs(BBJ_EHFINALLYRET)); + assert(fgPredsComputed); - // Don't `assert(succ->isBBCallFinallyPairTail())`; we've already unlinked the CALLFINALLY - assert(succ->KindIs(BBJ_CALLFINALLYRET)); + BBehfDesc* const ehfDesc = block->GetEhfTargets(); + const unsigned succCount = ehfDesc->bbeCount; + FlowEdge** succTab = ehfDesc->bbeSuccs; + assert(succIndex < succCount); + FlowEdge* succEdge = succTab[succIndex]; - fgRemoveRefPred(succ, block); + fgRemoveRefPred(succEdge); - BBehfDesc* const ehfDesc = block->GetEhfTargets(); - unsigned succCount = ehfDesc->bbeCount; - FlowEdge** succTab = ehfDesc->bbeSuccs; - bool found = false; + // If succEdge 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*)); + } + +#ifdef DEBUG + // We only expect to see a successor once in the table. + for (unsigned i = succIndex; i < (succCount - 1); i++) + { + assert(succTab[i]->getDestinationBlock() != succEdge->getDestinationBlock()); + } +#endif // DEBUG + + ehfDesc->bbeCount--; +} + +//------------------------------------------------------------------------ +// fgRemoveEhfSuccessor: Removes `succEdge` from its BBJ_EHFINALLYRET source block's jump table. +// Updates the predecessor list of the successor block, if necessary. +// +// Arguments: +// block - BBJ_EHFINALLYRET block +// succEdge - FlowEdge* to be removed from predecessor block's jump table +// +void Compiler::fgRemoveEhfSuccessor(FlowEdge* succEdge) +{ + assert(succEdge != nullptr); + assert(fgPredsComputed); + + BasicBlock* block = succEdge->getSourceBlock(); + assert(block != nullptr); + assert(block->KindIs(BBJ_EHFINALLYRET)); + + fgRemoveRefPred(succEdge); + + BBehfDesc* const ehfDesc = block->GetEhfTargets(); + const unsigned succCount = ehfDesc->bbeCount; + FlowEdge** succTab = ehfDesc->bbeSuccs; + bool found = false; - // Walk the successor table looking for the specified successor block. + // Search succTab for succEdge so we can splice it out of the table. for (unsigned i = 0; i < succCount; i++) { - if (succTab[i]->getDestinationBlock() == succ) + if (succTab[i] == succEdge) { - // If it's not the last one, move everything after in the table down one slot. - if (i + 1 < succCount) + // 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*)); } - --succCount; - found = true; #ifdef DEBUG // We only expect to see a successor once in the table. - for (; i < succCount; i++) + for (; i < (succCount - 1); i++) { - assert(succTab[i]->getDestinationBlock() != succ); + assert(succTab[i]->getDestinationBlock() != succEdge->getDestinationBlock()); } #endif // DEBUG - - break; } } - assert(found); - ehfDesc->bbeCount = succCount; + assert(found); + ehfDesc->bbeCount--; } //------------------------------------------------------------------------ @@ -766,6 +799,39 @@ void Compiler::fgReplacePred(BasicBlock* block, BasicBlock* oldPred, BasicBlock* } } +//------------------------------------------------------------------------ +// fgReplacePred: redirects the given edge to a new predecessor block +// +// Arguments: +// edge - the edge whose source block we want to update +// newPred - the new predecessor block for edge +// +// Notes: +// +// This function assumes that all branches from the predecessor (practically, that all +// switch cases that target the successor block) are changed to branch from the new predecessor, +// with the same dup count. +// +// Note that the successor block's bbRefs is not changed, since it has the same number of +// references as before, just from a different predecessor block. +// +// Also note this may cause sorting of the pred list. +// +void Compiler::fgReplacePred(FlowEdge* edge, BasicBlock* const newPred) +{ + assert(edge != nullptr); + assert(newPred != nullptr); + assert(edge->getSourceBlock() != newPred); + + edge->setSourceBlock(newPred); + + // We may now need to reorder the pred list. + // + BasicBlock* succBlock = edge->getDestinationBlock(); + assert(succBlock != nullptr); + succBlock->ensurePredListOrder(this); +} + /***************************************************************************** * For a block that is in a handler region, find the first block of the most-nested * handler containing the block. @@ -5365,9 +5431,9 @@ void Compiler::fgPrepareCallFinallyRetForRemoval(BasicBlock* block) // However, we might not have marked the BBJ_CALLFINALLY as BBF_RETLESS_CALL even though it is. // (Some early flow optimization should probably aggressively mark these as BBF_RETLESS_CALL // and not depend on fgRemoveBlock() to do that.) - for (BasicBlock* const leavePredBlock : block->PredBlocks()) + for (FlowEdge* leavePredEdge : block->PredEdges()) { - fgRemoveEhfSuccessor(leavePredBlock, block); + fgRemoveEhfSuccessor(leavePredEdge); } assert(block->bbRefs == 0); assert(block->bbPreds == nullptr); diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 3b6afa82bed54f..0fc963b6a693e0 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -339,6 +339,44 @@ FlowEdge* Compiler::fgRemoveRefPred(BasicBlock* block, BasicBlock* blockPred) } } +//------------------------------------------------------------------------ +// fgRemoveRefPred: Decrements the reference count of `edge`, removing it from its successor block's pred list +// if the reference count is zero. +// +// Arguments: +// edge -- The FlowEdge* to decrement the reference count of. +// +// Notes: +// -- succBlock->bbRefs is decremented by one to account for the reduction in incoming edges. +// -- fgModified is set if a flow edge is removed, indicating that the flow graph shape has changed. +// +void Compiler::fgRemoveRefPred(FlowEdge* edge) +{ + assert(edge != nullptr); + assert(fgPredsComputed); + + BasicBlock* predBlock = edge->getSourceBlock(); + BasicBlock* succBlock = edge->getDestinationBlock(); + assert(predBlock != nullptr); + assert(succBlock != nullptr); + + succBlock->bbRefs--; + + assert(edge->getDupCount() > 0); + edge->decrementDupCount(); + + if (edge->getDupCount() == 0) + { + // Splice out the predecessor edge in succBlock's pred list, since it's no longer necessary. + FlowEdge** ptrToPred; + FlowEdge* pred = fgGetPredForBlock(succBlock, predBlock, &ptrToPred); + *ptrToPred = pred->getNextPredEdge(); + + // Any changes to the flow graph invalidate the dominator sets. + fgModified = true; + } +} + //------------------------------------------------------------------------ // fgRemoveAllRefPreds: Removes a predecessor edge from one block to another, no matter what the "dup count" is. // @@ -406,11 +444,14 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block) break; case BBJ_EHFINALLYRET: - for (BasicBlock* const succ : block->EHFinallyRetSuccs()) + { + BBehfDesc* const ehfDesc = block->GetEhfTargets(); + for (unsigned i = 0; i < ehfDesc->bbeCount; i++) { - fgRemoveRefPred(succ, block); + fgRemoveRefPred(ehfDesc->bbeSuccs[i]); } break; + } case BBJ_EHFAULTRET: case BBJ_THROW: From a0d7271864eb579f2957390577a126e69e3c1401 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 15 Feb 2024 21:29:52 -0500 Subject: [PATCH 4/6] Inspiring edge usage in optRedirectBlock --- src/coreclr/jit/optimizer.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index f68a6afe02f057..55d021853782f2 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -644,24 +644,24 @@ void Compiler::optRedirectBlock(BasicBlock* blk, BasicBlock* newBlk, BlockToBloc BBehfDesc* newEhfDesc = new (this, CMK_BasicBlock) BBehfDesc; newEhfDesc->bbeCount = currEhfDesc->bbeCount; newEhfDesc->bbeSuccs = new (this, CMK_FlowEdge) FlowEdge*[newEhfDesc->bbeCount]; - FlowEdge** jumpPtr = newEhfDesc->bbeSuccs; - for (BasicBlock* const ehfTarget : blk->EHFinallyRetSuccs()) + for (unsigned i = 0; i < newEhfDesc->bbeCount; i++) { + FlowEdge* const inspiringEdge = currEhfDesc->bbeSuccs[i]; + BasicBlock* const ehfTarget = inspiringEdge->getDestinationBlock(); FlowEdge* newEdge; // Determine if newBlk should target ehfTarget, or be redirected if (redirectMap->Lookup(ehfTarget, &newTarget)) { - newEdge = fgAddRefPred(newTarget, newBlk); + newEdge = fgAddRefPred(newTarget, newBlk, inspiringEdge); } else { - newEdge = fgAddRefPred(ehfTarget, newBlk); + newEdge = fgAddRefPred(ehfTarget, newBlk, inspiringEdge); } - *jumpPtr = newEdge; - jumpPtr++; + newEhfDesc->bbeSuccs[i] = newEdge; } newBlk->SetEhf(newEhfDesc); From e75b3cdd1dad6a7013ed846f2f058bca5c5aae3a Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 16 Feb 2024 12:07:29 -0500 Subject: [PATCH 5/6] Add assert --- src/coreclr/jit/fgbasic.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 05ba42dedc2d3d..294ff36581ff06 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -487,13 +487,15 @@ void Compiler::fgReplaceEhfSuccessor(BasicBlock* block, BasicBlock* oldSucc, Bas unsigned newSuccNum = UINT_MAX; for (unsigned i = 0; i < succCount; i++) { + assert(succTab[i]->getSourceBlock() == block); + if (succTab[i]->getDestinationBlock() == newSucc) { assert(newSuccNum == UINT_MAX); newSuccNum = i; } - if (succTab[i]->getSourceBlock() == oldSucc) + if (succTab[i]->getDestinationBlock() == oldSucc) { assert(oldSuccNum == UINT_MAX); oldSuccNum = i; From 2ea522177ae02acf680c27ca7368f8b9093f520c Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 16 Feb 2024 12:08:08 -0500 Subject: [PATCH 6/6] Style --- src/coreclr/jit/fgbasic.cpp | 16 ++++++++-------- src/coreclr/jit/fgflow.cpp | 6 +++--- src/coreclr/jit/optimizer.cpp | 6 +++--- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 294ff36581ff06..3800e2ffe89d94 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -547,9 +547,9 @@ void Compiler::fgRemoveEhfSuccessor(BasicBlock* block, const unsigned succIndex) assert(block->KindIs(BBJ_EHFINALLYRET)); assert(fgPredsComputed); - BBehfDesc* const ehfDesc = block->GetEhfTargets(); - const unsigned succCount = ehfDesc->bbeCount; - FlowEdge** succTab = ehfDesc->bbeSuccs; + BBehfDesc* const ehfDesc = block->GetEhfTargets(); + const unsigned succCount = ehfDesc->bbeCount; + FlowEdge** succTab = ehfDesc->bbeSuccs; assert(succIndex < succCount); FlowEdge* succEdge = succTab[succIndex]; @@ -569,7 +569,7 @@ void Compiler::fgRemoveEhfSuccessor(BasicBlock* block, const unsigned succIndex) assert(succTab[i]->getDestinationBlock() != succEdge->getDestinationBlock()); } #endif // DEBUG - + ehfDesc->bbeCount--; } @@ -592,10 +592,10 @@ void Compiler::fgRemoveEhfSuccessor(FlowEdge* succEdge) fgRemoveRefPred(succEdge); - BBehfDesc* const ehfDesc = block->GetEhfTargets(); - const unsigned succCount = ehfDesc->bbeCount; - FlowEdge** succTab = ehfDesc->bbeSuccs; - bool found = false; + BBehfDesc* const ehfDesc = block->GetEhfTargets(); + const unsigned succCount = ehfDesc->bbeCount; + FlowEdge** succTab = ehfDesc->bbeSuccs; + bool found = false; // Search succTab for succEdge so we can splice it out of the table. for (unsigned i = 0; i < succCount; i++) diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 0fc963b6a693e0..a7766b64cae6bb 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -369,8 +369,8 @@ void Compiler::fgRemoveRefPred(FlowEdge* edge) { // Splice out the predecessor edge in succBlock's pred list, since it's no longer necessary. FlowEdge** ptrToPred; - FlowEdge* pred = fgGetPredForBlock(succBlock, predBlock, &ptrToPred); - *ptrToPred = pred->getNextPredEdge(); + FlowEdge* pred = fgGetPredForBlock(succBlock, predBlock, &ptrToPred); + *ptrToPred = pred->getNextPredEdge(); // Any changes to the flow graph invalidate the dominator sets. fgModified = true; @@ -444,7 +444,7 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block) break; case BBJ_EHFINALLYRET: - { + { BBehfDesc* const ehfDesc = block->GetEhfTargets(); for (unsigned i = 0; i < ehfDesc->bbeCount; i++) { diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 55d021853782f2..e2f1d335e21277 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -647,9 +647,9 @@ void Compiler::optRedirectBlock(BasicBlock* blk, BasicBlock* newBlk, BlockToBloc for (unsigned i = 0; i < newEhfDesc->bbeCount; i++) { - FlowEdge* const inspiringEdge = currEhfDesc->bbeSuccs[i]; - BasicBlock* const ehfTarget = inspiringEdge->getDestinationBlock(); - FlowEdge* newEdge; + FlowEdge* const inspiringEdge = currEhfDesc->bbeSuccs[i]; + BasicBlock* const ehfTarget = inspiringEdge->getDestinationBlock(); + FlowEdge* newEdge; // Determine if newBlk should target ehfTarget, or be redirected if (redirectMap->Lookup(ehfTarget, &newTarget))