From 87273d64859e8bf5856d176915624e6f92ed3baf Mon Sep 17 00:00:00 2001 From: Aman Khalid Date: Wed, 9 Oct 2024 14:55:02 +0000 Subject: [PATCH] JIT: Run single EH region repair pass after layout (#108634) Fixes #108608. To simplify EH maintenance logic, leave EH regions in a deconstructed phase during block reordering, and correct the end block pointers of each region in a pass over the block list after. By walking the list backwards, we can short-circuit finding the end of each region, and we don't run into the issue of an EH region not being able to determine if it's at the end of its parent region due to some awkward placement of a sibling region. --- src/coreclr/jit/compiler.cpp | 6 ++ src/coreclr/jit/compiler.h | 5 +- src/coreclr/jit/fgopt.cpp | 178 ++--------------------------------- src/coreclr/jit/jiteh.cpp | 89 ++++++++++++++++++ 4 files changed, 103 insertions(+), 175 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 3b111c4572b97..4f67fae05e3e2 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5257,6 +5257,12 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl auto lateLayoutPhase = [this] { fgDoReversePostOrderLayout(); fgMoveColdBlocks(); + + if (compHndBBtabCount != 0) + { + fgFindEHRegionEnds(); + } + return PhaseStatus::MODIFIED_EVERYTHING; }; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index af5bf6b4e9bac..760423f789762 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2832,9 +2832,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. @@ -2939,6 +2936,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 fd7c8d6ea2514..b63dee7139938 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3346,6 +3346,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 // @@ -4691,22 +4696,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 @@ -4727,9 +4716,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()) @@ -4776,89 +4762,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; - } - } - } } //----------------------------------------------------------------------------- @@ -5041,7 +4944,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++) { @@ -5087,75 +4990,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); - } - } } } diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 98c0922beb7ff..67b404d4016ca 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -1253,6 +1253,95 @@ 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); + 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, &unsetTryEnds](BasicBlock* block) { + for (unsigned tryIndex = block->getTryIndex(); tryIndex != EHblkDsc::NO_ENCLOSING_INDEX; + tryIndex = ehGetEnclosingTryIndex(tryIndex)) + { + EHblkDsc* const HBtab = ehGetDsc(tryIndex); + if (HBtab->ebdTryLast == nullptr) + { + assert(unsetTryEnds != 0); + HBtab->ebdTryLast = block; + 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, &unsetHndEnds](BasicBlock* block) { + for (unsigned hndIndex = block->getHndIndex(); hndIndex != EHblkDsc::NO_ENCLOSING_INDEX; + hndIndex = ehGetEnclosingHndIndex(hndIndex)) + { + EHblkDsc* const HBtab = ehGetDsc(hndIndex); + if (HBtab->ebdHndLast == nullptr) + { + assert(unsetHndEnds != 0); + HBtab->ebdHndLast = block; + 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