From 27d8ca1055459ccaf6bfa2e4799bc274fec15e4a Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 7 Oct 2024 21:18:48 -0400 Subject: [PATCH 1/4] Add EH region ends pass --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/fgopt.cpp | 5 +++ src/coreclr/jit/jiteh.cpp | 85 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 69214ae889568..0a3eac7d427b4 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2938,6 +2938,8 @@ class Compiler void fgSetHndEnd(EHblkDsc* handlerTab, BasicBlock* newHndLast); + void fgFindEHRegionEnds(); + void fgSkipRmvdBlocks(EHblkDsc* handlerTab); void fgAllocEHTable(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 22b1e7127a36e..02d9d814d4804 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3357,6 +3357,11 @@ bool Compiler::fgReorderBlocks(bool useProfile) fgDoReversePostOrderLayout(); fgMoveColdBlocks(); + if (compHndBBtabCount != 0) + { + fgFindEHRegionEnds(); + } + // Renumber blocks to facilitate LSRA's order of block visitation // TODO: Consider removing this, and using traversal order in lSRA // diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 2270d5b165636..5e0bad51d68db 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -1302,6 +1302,91 @@ void Compiler::fgSetHndEnd(EHblkDsc* handlerTab, BasicBlock* newHndLast) } } +//------------------------------------------------------------- +// fgFindEHRegionEnds: Walk the block list, and set each try/handler region's end block. +// +void Compiler::fgFindEHRegionEnds() +{ + assert(compHndBBtabCount != 0); + bool* const tryEndsSet = new (this, CMK_Generic) bool[compHndBBtabCount * 2]{}; + bool* const hndEndsSet = tryEndsSet + compHndBBtabCount; + unsigned unsetTryEnds = compHndBBtabCount; + unsigned unsetHndEnds = compHndBBtabCount; + + // Updates the try region's (and all of its parent regions') end block to 'block,' + // if the try region's end block hasn't been updated yet. + auto setTryEnd = [this, tryEndsSet, &unsetTryEnds](BasicBlock* block) { + for (unsigned tryIndex = block->getTryIndex(); tryIndex != EHblkDsc::NO_ENCLOSING_INDEX; + tryIndex = ehGetEnclosingTryIndex(tryIndex)) + { + if (!tryEndsSet[tryIndex]) + { + assert(unsetTryEnds != 0); + ehGetDsc(tryIndex)->ebdTryLast = block; + tryEndsSet[tryIndex] = true; + unsetTryEnds--; + + } + else + { + break; + } + } + }; + + // Updates the handler region's (and all of its parent regions') end block to 'block,' + // if the handler region's end block hasn't been updated yet. + auto setHndEnd = [this, hndEndsSet, &unsetHndEnds](BasicBlock* block) { + for (unsigned hndIndex = block->getHndIndex(); hndIndex != EHblkDsc::NO_ENCLOSING_INDEX; + hndIndex = ehGetEnclosingHndIndex(hndIndex)) + { + if (!hndEndsSet[hndIndex]) + { + assert(unsetHndEnds != 0); + ehGetDsc(hndIndex)->ebdHndLast = block; + hndEndsSet[hndIndex] = true; + unsetHndEnds--; + } + else + { + break; + } + } + }; + + // Iterate backwards through the main method body, and update each try region's end block + for (BasicBlock* block = fgLastBBInMainFunction(); (unsetTryEnds != 0) && (block != nullptr); + block = block->Prev()) + { + if (block->hasTryIndex()) + { + setTryEnd(block); + } + } + + // If we don't have a funclet section, then all of the try regions should have been updated above + assert((unsetTryEnds == 0) || (fgFirstFuncletBB != nullptr)); + + // If we do have a funclet section, update the ends of any try regions nested in funclets + for (BasicBlock* block = fgLastBB; (unsetTryEnds != 0) && (block != fgLastBBInMainFunction()); + block = block->Prev()) + { + if (block->hasTryIndex()) + { + setTryEnd(block); + } + } + + // Finally, update the handler regions' ends + for (BasicBlock* block = fgLastBB; (unsetHndEnds != 0) && (block != nullptr); block = block->Prev()) + { + if (block->hasHndIndex()) + { + setHndEnd(block); + } + } +} + /***************************************************************************** * * Given a EH handler table entry update the ebdTryLast and ebdHndLast pointers From 85646f517ec56e80a647aa8bc00c5cffe0b33c4b Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 7 Oct 2024 21:39:44 -0400 Subject: [PATCH 2/4] Remove old EH fixup logic --- src/coreclr/jit/compiler.h | 3 - src/coreclr/jit/fgopt.cpp | 173 +------------------------------------ 2 files changed, 1 insertion(+), 175 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 0a3eac7d427b4..51240f7ec8d52 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2829,9 +2829,6 @@ class Compiler EHblkDsc* ehIsBlockHndLast(BasicBlock* block); bool ehIsBlockEHLast(BasicBlock* block); - template - void ehUpdateTryLasts(GetTryLast getTryLast, SetTryLast setTryLast); - bool ehBlockHasExnFlowDsc(BasicBlock* block); // Return the region index of the most nested EH region this block is in. diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 02d9d814d4804..d3fcd3914a5fb 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4707,22 +4707,6 @@ void Compiler::fgDoReversePostOrderLayout() return; } - // The RPO will scramble the EH regions, requiring us to correct their state. - // To do so, we will need to determine the new end blocks of each region. - // - struct EHLayoutInfo - { - BasicBlock* tryRegionEnd; - BasicBlock* hndRegionEnd; - bool tryRegionInMainBody; - - // Default constructor provided so we can call ArrayStack::Emplace - // - EHLayoutInfo() = default; - }; - - ArrayStack regions(getAllocator(CMK_ArrayStack), compHndBBtabCount); - // The RPO will break up call-finally pairs, so save them before re-ordering // struct CallFinallyPair @@ -4743,9 +4727,6 @@ void Compiler::fgDoReversePostOrderLayout() for (EHblkDsc* const HBtab : EHClauses(this)) { - // Default-initialize a EHLayoutInfo for each EH clause - regions.Emplace(); - if (HBtab->HasFinallyHandler()) { for (BasicBlock* const pred : HBtab->ebdHndBeg->PredBlocks()) @@ -4792,89 +4773,6 @@ void Compiler::fgDoReversePostOrderLayout() } fgMoveHotJumps(); - - // The RPO won't change the entry blocks of any EH regions, but reordering can change the last block in a region - // (for example, by pushing throw blocks unreachable via normal flow to the end of the region). - // First, determine the new EH region ends. - // - - for (BasicBlock* const block : Blocks(fgFirstBB, fgLastBBInMainFunction())) - { - if (block->hasTryIndex()) - { - EHLayoutInfo& layoutInfo = regions.BottomRef(block->getTryIndex()); - layoutInfo.tryRegionEnd = block; - layoutInfo.tryRegionInMainBody = true; - } - - if (block->hasHndIndex()) - { - regions.BottomRef(block->getHndIndex()).hndRegionEnd = block; - } - } - - for (BasicBlock* const block : Blocks(fgFirstFuncletBB)) - { - if (block->hasHndIndex()) - { - regions.BottomRef(block->getHndIndex()).hndRegionEnd = block; - } - - if (block->hasTryIndex()) - { - EHLayoutInfo& layoutInfo = regions.BottomRef(block->getTryIndex()); - - if (!layoutInfo.tryRegionInMainBody) - { - layoutInfo.tryRegionEnd = block; - } - } - } - - // Now, update the EH descriptors, starting with the try regions - // - auto getTryLast = [®ions](const unsigned index) -> BasicBlock* { - return regions.BottomRef(index).tryRegionEnd; - }; - - auto setTryLast = [®ions](const unsigned index, BasicBlock* const block) { - regions.BottomRef(index).tryRegionEnd = block; - }; - - ehUpdateTryLasts(getTryLast, setTryLast); - - // Now, do the handler regions - // - unsigned XTnum = 0; - for (EHblkDsc* const HBtab : EHClauses(this)) - { - // The end of each handler region should have been visited by iterating the blocklist above - // - BasicBlock* const hndEnd = regions.BottomRef(XTnum++).hndRegionEnd; - assert(hndEnd != nullptr); - - // Update the end pointer of this handler region to the new last block - // - HBtab->ebdHndLast = hndEnd; - const unsigned enclosingHndIndex = HBtab->ebdEnclosingHndIndex; - - // If this handler region is nested in another one, we might need to update its enclosing region's end block - // - if (enclosingHndIndex != EHblkDsc::NO_ENCLOSING_INDEX) - { - BasicBlock* const enclosingHndEnd = regions.BottomRef(enclosingHndIndex).hndRegionEnd; - assert(enclosingHndEnd != nullptr); - - // If the enclosing region ends right before the nested region begins, - // extend the enclosing region's last block to the end of the nested region. - // - BasicBlock* const hndBeg = HBtab->HasFilter() ? HBtab->ebdFilter : HBtab->ebdHndBeg; - if (enclosingHndEnd->NextIs(hndBeg)) - { - regions.BottomRef(enclosingHndIndex).hndRegionEnd = hndEnd; - } - } - } } //----------------------------------------------------------------------------- @@ -5057,7 +4955,7 @@ void Compiler::fgMoveColdBlocks() } } - // Before updating EH descriptors, find the new try region ends + // Find the new try region ends // for (unsigned XTnum = 0; XTnum < compHndBBtabCount; XTnum++) { @@ -5103,75 +5001,6 @@ void Compiler::fgMoveColdBlocks() fgInsertBBafter(newTryEnd, prev); } } - else - { - // Otherwise, just update the try region end - // - tryRegionEnds[XTnum] = newTryEnd; - } - } - - // Now, update EH descriptors - // - auto getTryLast = [tryRegionEnds](const unsigned index) -> BasicBlock* { - return tryRegionEnds[index]; - }; - - auto setTryLast = [tryRegionEnds](const unsigned index, BasicBlock* const block) { - tryRegionEnds[index] = block; - }; - - ehUpdateTryLasts(getTryLast, setTryLast); -} - -//------------------------------------------------------------- -// ehUpdateTryLasts: Iterates EH descriptors, updating each try region's -// end block as determined by getTryLast. -// -// Type parameters: -// GetTryLast - Functor type that takes an EH index, -// and returns the corresponding region's new try end block -// SetTryLast - Functor type that takes an EH index and a BasicBlock*, -// and updates some internal state tracking the new try end block of each EH region -// -// Parameters: -// getTryLast - Functor to get new try end block for an EH region -// setTryLast - Functor to update the new try end block for an EH region -// -template -void Compiler::ehUpdateTryLasts(GetTryLast getTryLast, SetTryLast setTryLast) -{ - unsigned XTnum = 0; - for (EHblkDsc* const HBtab : EHClauses(this)) - { - BasicBlock* const tryEnd = getTryLast(XTnum++); - - if (tryEnd == nullptr) - { - continue; - } - - // Update the end pointer of this try region to the new last block - // - HBtab->ebdTryLast = tryEnd; - const unsigned enclosingTryIndex = HBtab->ebdEnclosingTryIndex; - - // If this try region is nested in another one, we might need to update its enclosing region's end block - // - if (enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX) - { - BasicBlock* const enclosingTryEnd = getTryLast(enclosingTryIndex); - - // If multiple EH descriptors map to the same try region, - // then the enclosing region's last block might be null in the table, so set it here. - // Similarly, if the enclosing region ends right before the nested region begins, - // extend the enclosing region's last block to the end of the nested region. - // - if ((enclosingTryEnd == nullptr) || enclosingTryEnd->NextIs(HBtab->ebdTryBeg)) - { - setTryLast(enclosingTryIndex, tryEnd); - } - } } } From 1bb52ec4071b20e3184097479da3e87adff47edf Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 7 Oct 2024 21:50:26 -0400 Subject: [PATCH 3/4] Style --- src/coreclr/jit/jiteh.cpp | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 5e0bad51d68db..35099ece584c8 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -1308,24 +1308,23 @@ void Compiler::fgSetHndEnd(EHblkDsc* handlerTab, BasicBlock* newHndLast) void Compiler::fgFindEHRegionEnds() { assert(compHndBBtabCount != 0); - bool* const tryEndsSet = new (this, CMK_Generic) bool[compHndBBtabCount * 2]{}; - bool* const hndEndsSet = tryEndsSet + compHndBBtabCount; - unsigned unsetTryEnds = compHndBBtabCount; - unsigned unsetHndEnds = compHndBBtabCount; + bool* const tryEndsSet = new (this, CMK_Generic) bool[compHndBBtabCount * 2]{}; + bool* const hndEndsSet = tryEndsSet + compHndBBtabCount; + unsigned unsetTryEnds = compHndBBtabCount; + unsigned unsetHndEnds = compHndBBtabCount; // Updates the try region's (and all of its parent regions') end block to 'block,' // if the try region's end block hasn't been updated yet. auto setTryEnd = [this, tryEndsSet, &unsetTryEnds](BasicBlock* block) { for (unsigned tryIndex = block->getTryIndex(); tryIndex != EHblkDsc::NO_ENCLOSING_INDEX; - tryIndex = ehGetEnclosingTryIndex(tryIndex)) + tryIndex = ehGetEnclosingTryIndex(tryIndex)) { if (!tryEndsSet[tryIndex]) { assert(unsetTryEnds != 0); ehGetDsc(tryIndex)->ebdTryLast = block; - tryEndsSet[tryIndex] = true; + tryEndsSet[tryIndex] = true; unsetTryEnds--; - } else { @@ -1338,13 +1337,13 @@ void Compiler::fgFindEHRegionEnds() // if the handler region's end block hasn't been updated yet. auto setHndEnd = [this, hndEndsSet, &unsetHndEnds](BasicBlock* block) { for (unsigned hndIndex = block->getHndIndex(); hndIndex != EHblkDsc::NO_ENCLOSING_INDEX; - hndIndex = ehGetEnclosingHndIndex(hndIndex)) + hndIndex = ehGetEnclosingHndIndex(hndIndex)) { if (!hndEndsSet[hndIndex]) { assert(unsetHndEnds != 0); ehGetDsc(hndIndex)->ebdHndLast = block; - hndEndsSet[hndIndex] = true; + hndEndsSet[hndIndex] = true; unsetHndEnds--; } else @@ -1355,8 +1354,7 @@ void Compiler::fgFindEHRegionEnds() }; // Iterate backwards through the main method body, and update each try region's end block - for (BasicBlock* block = fgLastBBInMainFunction(); (unsetTryEnds != 0) && (block != nullptr); - block = block->Prev()) + for (BasicBlock* block = fgLastBBInMainFunction(); (unsetTryEnds != 0) && (block != nullptr); block = block->Prev()) { if (block->hasTryIndex()) { @@ -1369,7 +1367,7 @@ void Compiler::fgFindEHRegionEnds() // If we do have a funclet section, update the ends of any try regions nested in funclets for (BasicBlock* block = fgLastBB; (unsetTryEnds != 0) && (block != fgLastBBInMainFunction()); - block = block->Prev()) + block = block->Prev()) { if (block->hasTryIndex()) { From d254188e47e89ed1268844d2c9604428f390e9e4 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 8 Oct 2024 11:32:53 -0400 Subject: [PATCH 4/4] Use null EH clause pointers as set indicators --- src/coreclr/jit/jiteh.cpp | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 35099ece584c8..fa76797d5340c 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -1308,22 +1308,28 @@ void Compiler::fgSetHndEnd(EHblkDsc* handlerTab, BasicBlock* newHndLast) void Compiler::fgFindEHRegionEnds() { assert(compHndBBtabCount != 0); - bool* const tryEndsSet = new (this, CMK_Generic) bool[compHndBBtabCount * 2]{}; - bool* const hndEndsSet = tryEndsSet + compHndBBtabCount; - unsigned unsetTryEnds = compHndBBtabCount; - unsigned unsetHndEnds = compHndBBtabCount; + unsigned unsetTryEnds = compHndBBtabCount; + unsigned unsetHndEnds = compHndBBtabCount; + + // Null out each clause's end pointers. + // A non-null end pointer indicates we already updated the clause. + for (EHblkDsc* const HBtab : EHClauses(this)) + { + HBtab->ebdTryLast = nullptr; + HBtab->ebdHndLast = nullptr; + } // Updates the try region's (and all of its parent regions') end block to 'block,' // if the try region's end block hasn't been updated yet. - auto setTryEnd = [this, tryEndsSet, &unsetTryEnds](BasicBlock* block) { + auto setTryEnd = [this, &unsetTryEnds](BasicBlock* block) { for (unsigned tryIndex = block->getTryIndex(); tryIndex != EHblkDsc::NO_ENCLOSING_INDEX; tryIndex = ehGetEnclosingTryIndex(tryIndex)) { - if (!tryEndsSet[tryIndex]) + EHblkDsc* const HBtab = ehGetDsc(tryIndex); + if (HBtab->ebdTryLast == nullptr) { assert(unsetTryEnds != 0); - ehGetDsc(tryIndex)->ebdTryLast = block; - tryEndsSet[tryIndex] = true; + HBtab->ebdTryLast = block; unsetTryEnds--; } else @@ -1335,15 +1341,15 @@ void Compiler::fgFindEHRegionEnds() // Updates the handler region's (and all of its parent regions') end block to 'block,' // if the handler region's end block hasn't been updated yet. - auto setHndEnd = [this, hndEndsSet, &unsetHndEnds](BasicBlock* block) { + auto setHndEnd = [this, &unsetHndEnds](BasicBlock* block) { for (unsigned hndIndex = block->getHndIndex(); hndIndex != EHblkDsc::NO_ENCLOSING_INDEX; hndIndex = ehGetEnclosingHndIndex(hndIndex)) { - if (!hndEndsSet[hndIndex]) + EHblkDsc* const HBtab = ehGetDsc(hndIndex); + if (HBtab->ebdHndLast == nullptr) { assert(unsetHndEnds != 0); - ehGetDsc(hndIndex)->ebdHndLast = block; - hndEndsSet[hndIndex] = true; + HBtab->ebdHndLast = block; unsetHndEnds--; } else