Skip to content

Commit

Permalink
JIT: Run single EH region repair pass after layout (dotnet#108634)
Browse files Browse the repository at this point in the history 
Fixes dotnet#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.
  • Loading branch information
@amanasifkhalid
amanasifkhalid authored Oct 9, 2024
1 parent 7318855 commit 87273d6
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 175 deletions.
6 changes: 6 additions & 0 deletions src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down
5 changes: 2 additions & 3 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2832,9 +2832,6 @@ class Compiler
EHblkDsc* ehIsBlockHndLast(BasicBlock* block);
bool ehIsBlockEHLast(BasicBlock* block);

template <typename GetTryLast, typename SetTryLast>
void ehUpdateTryLasts(GetTryLast getTryLast, SetTryLast setTryLast);

bool ehBlockHasExnFlowDsc(BasicBlock* block);

// Return the region index of the most nested EH region this block is in.
Expand Down Expand Up @@ -2939,6 +2936,8 @@ class Compiler

void fgSetHndEnd(EHblkDsc* handlerTab, BasicBlock* newHndLast);

void fgFindEHRegionEnds();

void fgSkipRmvdBlocks(EHblkDsc* handlerTab);

void fgAllocEHTable();
Expand Down
178 changes: 6 additions & 172 deletions src/coreclr/jit/fgopt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
//
Expand Down Expand Up @@ -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<EHLayoutInfo> regions(getAllocator(CMK_ArrayStack), compHndBBtabCount);

// The RPO will break up call-finally pairs, so save them before re-ordering
//
struct CallFinallyPair
Expand All @@ -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())
Expand Down Expand Up @@ -4776,89 +4762,6 @@ void Compiler::fgDoReversePostOrderLayout()
}

fgMoveHotJumps</* hasEH */ true>();

// 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 = [&regions](const unsigned index) -> BasicBlock* {
return regions.BottomRef(index).tryRegionEnd;
};

auto setTryLast = [&regions](const unsigned index, BasicBlock* const block) {
regions.BottomRef(index).tryRegionEnd = block;
};

ehUpdateTryLasts<decltype(getTryLast), decltype(setTryLast)>(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;
}
}
}
}

//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -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++)
{
Expand Down Expand Up @@ -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<decltype(getTryLast), decltype(setTryLast)>(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 <typename GetTryLast, typename SetTryLast>
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);
}
}
}
}

Expand Down
89 changes: 89 additions & 0 deletions src/coreclr/jit/jiteh.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 87273d6

Please sign in to comment.