From 7536db85ade8d8b59f03750101a772bed83a50fa Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 9 Sep 2024 19:36:34 -0400 Subject: [PATCH] Simplify finally cloning logic --- src/coreclr/jit/compiler.h | 6 +++- src/coreclr/jit/fgbasic.cpp | 14 ++------ src/coreclr/jit/fgehopt.cpp | 50 ++++++++++----------------- src/coreclr/jit/fgopt.cpp | 8 ++--- src/coreclr/jit/jiteh.cpp | 68 ++++++++++++++----------------------- 5 files changed, 56 insertions(+), 90 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a23a650a805f25..6f46e652b0293f 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2833,7 +2833,7 @@ class Compiler bool ehIsBlockEHLast(BasicBlock* block); template - void ehUpdateTryLasts(GetTryLast getTryLast, SetTryLast setTryLast); + void ehSetTryLasts(GetTryLast getTryLast, SetTryLast setTryLast); bool ehBlockHasExnFlowDsc(BasicBlock* block); @@ -2858,6 +2858,10 @@ class Compiler // Update the 'last' pointers in the EH table to reflect new or deleted blocks in an EH region. void ehUpdateLastBlocks(BasicBlock* oldLast, BasicBlock* newLast); + // Update the end pointer of the try region containing 'oldTryLast', + // as well as the end pointers of any parent try regions, to 'newTryLast'. + void ehUpdateLastTryBlocks(BasicBlock* oldTryLast, BasicBlock* newTryLast); + // For a finally handler, find the region index that the BBJ_CALLFINALLY lives in that calls the handler, // or NO_ENCLOSING_INDEX if the BBJ_CALLFINALLY lives in the main function body. Normally, the index // is the same index as the handler (and the BBJ_CALLFINALLY lives in the 'try' region), but for AMD64 the diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 774e456f5e761a..9193ad14d87bc7 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -6736,23 +6736,15 @@ BasicBlock* Compiler::fgNewBBinRegionWorker(BBKinds jumpKind, // BasicBlock* Compiler::fgNewBBatTryRegionEnd(BBKinds jumpKind, unsigned tryIndex) { - BasicBlock* const oldTryLast = ehGetDsc(tryIndex)->ebdTryLast; + EHblkDsc* HBtab = ehGetDsc(tryIndex); + BasicBlock* const oldTryLast = HBtab->ebdTryLast; BasicBlock* const newBlock = fgNewBBafter(jumpKind, oldTryLast, /* extendRegion */ false); newBlock->setTryIndex(tryIndex); newBlock->clearHndIndex(); // Update this try region's (and all parent try regions') last block pointer // - for (unsigned XTnum = tryIndex; XTnum < compHndBBtabCount; XTnum++) - { - EHblkDsc* const HBtab = ehGetDsc(XTnum); - if (HBtab->ebdTryLast == oldTryLast) - { - assert((XTnum == tryIndex) || (ehGetDsc(tryIndex)->ebdEnclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX)); - fgSetTryEnd(HBtab, newBlock); - } - } - + ehUpdateLastTryBlocks(oldTryLast, newBlock); return newBlock; } diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 743559717e47cb..6bd050183fcc9d 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -1009,44 +1009,18 @@ PhaseStatus Compiler::fgCloneFinally() // Clone the finally body, and splice it into the flow graph // within the parent region of the try. // - const unsigned finallyTryIndex = firstBlock->bbTryIndex; - BasicBlock* insertAfter = nullptr; BlockToBlockMap blockMap(getAllocator()); unsigned cloneBBCount = 0; weight_t const originalWeight = firstBlock->hasProfileWeight() ? firstBlock->bbWeight : BB_ZERO_WEIGHT; + bool reachedEnd = false; - for (BasicBlock* block = firstBlock; block != nextBlock; block = block->Next()) + for (BasicBlock* block = firstBlock; !reachedEnd; block = block->Next()) { - BasicBlock* newBlock; - - if (block == firstBlock) - { - // Put first cloned finally block into the appropriate - // region, somewhere within or after the range of - // callfinallys, depending on the EH implementation. - const unsigned hndIndex = 0; - BasicBlock* const nearBlk = cloneInsertAfter; - newBlock = fgNewBBinRegion(BBJ_ALWAYS, finallyTryIndex, hndIndex, nearBlk); - - // If the clone ends up just after the finally, adjust - // the stopping point for finally traversal. - if (newBlock->NextIs(nextBlock)) - { - assert(newBlock->PrevIs(lastBlock)); - nextBlock = newBlock; - } - } - else - { - // Put subsequent blocks in the same region... - const bool extendRegion = true; - newBlock = fgNewBBafter(BBJ_ALWAYS, insertAfter, extendRegion); - } - + BasicBlock* newBlock = fgNewBBafter(BBJ_ALWAYS, cloneInsertAfter, /* extendRegion */ false); + cloneInsertAfter = newBlock; cloneBBCount++; assert(cloneBBCount <= regionBBCount); - insertAfter = newBlock; blockMap.Set(block, newBlock); BasicBlock::CloneBlockState(this, newBlock, block); @@ -1062,10 +1036,11 @@ PhaseStatus Compiler::fgCloneFinally() { // Mark the block as the end of the cloned finally. newBlock->SetFlags(BBF_CLONED_FINALLY_END); + reachedEnd = true; } // Make sure clone block state hasn't munged the try region. - assert(newBlock->bbTryIndex == finallyTryIndex); + assert(newBlock->bbTryIndex == firstBlock->bbTryIndex); // Cloned handler block is no longer within the handler. newBlock->clearHndIndex(); @@ -1075,6 +1050,17 @@ PhaseStatus Compiler::fgCloneFinally() assert(!newBlock->HasInitializedTarget()); } + // If the cloned blocks were inserted at the end of a try region, update the region's end pointer + if (firstBlock->hasTryIndex()) + { + BasicBlock* const oldLastTry = ehGetDsc(firstBlock->getTryIndex())->ebdTryLast; + if (!oldLastTry->IsLast() && BasicBlock::sameTryRegion(oldLastTry, oldLastTry->Next())) + { + assert(oldLastTry->NextIs(blockMap[firstBlock])); + ehUpdateLastTryBlocks(oldLastTry, blockMap[lastBlock]); + } + } + // We should have cloned all the finally region blocks. assert(cloneBBCount == regionBBCount); @@ -1084,7 +1070,7 @@ PhaseStatus Compiler::fgCloneFinally() // Redirect any branches within the newly-cloned // finally, and any finally returns to jump to the return // point. - for (BasicBlock* block = firstBlock; block != nextBlock; block = block->Next()) + for (BasicBlock* block = firstBlock; !lastBlock->NextIs(block); block = block->Next()) { BasicBlock* newBlock = blockMap[block]; // Jump kind/target should not be set yet diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 5a71d474b6f365..333fb1513996ee 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4828,7 +4828,7 @@ void Compiler::fgDoReversePostOrderLayout() regions.BottomRef(index).tryRegionEnd = block; }; - ehUpdateTryLasts(getTryLast, setTryLast); + ehSetTryLasts(getTryLast, setTryLast); // Now, do the handler regions // @@ -5108,11 +5108,11 @@ void Compiler::fgMoveColdBlocks() tryRegionEnds[index] = block; }; - ehUpdateTryLasts(getTryLast, setTryLast); + ehSetTryLasts(getTryLast, setTryLast); } //------------------------------------------------------------- -// ehUpdateTryLasts: Iterates EH descriptors, updating each try region's +// ehSetTryLasts: Iterates EH descriptors, updating each try region's // end block as determined by getTryLast. // // Type parameters: @@ -5126,7 +5126,7 @@ void Compiler::fgMoveColdBlocks() // setTryLast - Functor to update the new try end block for an EH region // template -void Compiler::ehUpdateTryLasts(GetTryLast getTryLast, SetTryLast setTryLast) +void Compiler::ehSetTryLasts(GetTryLast getTryLast, SetTryLast setTryLast) { unsigned XTnum = 0; for (EHblkDsc* const HBtab : EHClauses(this)) diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 3ba4e73353db1e..92ba7a1f180d8d 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -901,6 +901,26 @@ void Compiler::ehUpdateLastBlocks(BasicBlock* oldLast, BasicBlock* newLast) } } +//----------------------------------------------------------------------------- +// ehUpdateLastTryBlocks: Update the end pointer of the try region containing 'oldTryLast', +// as well as the end pointers of any parent try regions, to 'newTryLast'. +// +// Arguments: +// oldTryLast - The previous end block of the try region(s) to be updated +// newTryLast - The new end block +// +void Compiler::ehUpdateLastTryBlocks(BasicBlock* oldTryLast, BasicBlock* newTryLast) +{ + assert(oldTryLast->hasTryIndex() && BasicBlock::sameTryRegion(oldTryLast, newTryLast)); + unsigned XTnum = oldTryLast->getTryIndex(); + for (EHblkDsc* HBtab = ehGetDsc(XTnum); (XTnum < compHndBBtabCount) && (HBtab->ebdTryLast == oldTryLast); + XTnum++, HBtab++) + { + assert((XTnum == oldTryLast->getTryIndex()) || (ehGetDsc(XTnum - 1)->ebdEnclosingTryIndex == XTnum)); + fgSetTryEnd(HBtab, newTryLast); + } +} + unsigned Compiler::ehGetCallFinallyRegionIndex(unsigned finallyIndex, bool* inTryRegion) { assert(finallyIndex != EHblkDsc::NO_ENCLOSING_INDEX); @@ -1239,16 +1259,8 @@ EHblkDsc* Compiler::ehInitTryBlockRange(BasicBlock* blk, BasicBlock** tryBeg, Ba void Compiler::fgSetTryBeg(EHblkDsc* handlerTab, BasicBlock* newTryBeg) { assert(newTryBeg != nullptr); - - // Check if we are going to change the existing value of endTryLast - // - if (handlerTab->ebdTryBeg != newTryBeg) - { - // Update the EH table with the newTryLast block - handlerTab->ebdTryBeg = newTryBeg; - - JITDUMP("EH#%u: New first block of try: " FMT_BB "\n", ehGetIndex(handlerTab), handlerTab->ebdTryBeg->bbNum); - } + handlerTab->ebdTryBeg = newTryBeg; + JITDUMP("EH#%u: New first block of try: " FMT_BB "\n", ehGetIndex(handlerTab), newTryBeg->bbNum); } /***************************************************************************** @@ -1258,22 +1270,8 @@ void Compiler::fgSetTryBeg(EHblkDsc* handlerTab, BasicBlock* newTryBeg) void Compiler::fgSetTryEnd(EHblkDsc* handlerTab, BasicBlock* newTryLast) { assert(newTryLast != nullptr); - - // - // Check if we are going to change the existing value of endTryLast - // - if (handlerTab->ebdTryLast != newTryLast) - { - // Update the EH table with the newTryLast block - handlerTab->ebdTryLast = newTryLast; - -#ifdef DEBUG - if (verbose) - { - printf("EH#%u: New last block of try: " FMT_BB "\n", ehGetIndex(handlerTab), newTryLast->bbNum); - } -#endif // DEBUG - } + handlerTab->ebdTryLast = newTryLast; + JITDUMP("EH#%u: New last block of try: " FMT_BB "\n", ehGetIndex(handlerTab), newTryLast->bbNum); } /***************************************************************************** @@ -1284,22 +1282,8 @@ void Compiler::fgSetTryEnd(EHblkDsc* handlerTab, BasicBlock* newTryLast) void Compiler::fgSetHndEnd(EHblkDsc* handlerTab, BasicBlock* newHndLast) { assert(newHndLast != nullptr); - - // - // Check if we are going to change the existing value of endHndLast - // - if (handlerTab->ebdHndLast != newHndLast) - { - // Update the EH table with the newHndLast block - handlerTab->ebdHndLast = newHndLast; - -#ifdef DEBUG - if (verbose) - { - printf("EH#%u: New last block of handler: " FMT_BB "\n", ehGetIndex(handlerTab), newHndLast->bbNum); - } -#endif // DEBUG - } + handlerTab->ebdHndLast = newHndLast; + JITDUMP("EH#%u: New last block of handler: " FMT_BB "\n", ehGetIndex(handlerTab), newHndLast->bbNum); } /*****************************************************************************