Skip to content

JIT: Fix placement of loop exit blocks in handler regions #107582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 10, 2024
Merged

JIT: Fix placement of loop exit blocks in handler regions #107582

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
42c4470
NOMERGE dump flowgraph in CI
amanasifkhalid Sep 9, 2024
e1b3622
Set handler index of exit block, if necessary
amanasifkhalid Sep 10, 2024
26cb4e8
Fix assert
amanasifkhalid Sep 10, 2024
03ac05a
Update handler region ends in fgNewBBatTryRegionEnd
amanasifkhalid Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions src/coreclr/jit/fgbasic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6725,7 +6725,7 @@ BasicBlock* Compiler::fgNewBBinRegionWorker(BBKinds jumpKind,

//-----------------------------------------------------------------------------
// fgNewBBatTryRegionEnd: Creates and inserts a new block at the end of the specified
// try region, updating the try end pointers in the EH table as necessary.
// try region, updating the end pointers in the EH table as necessary.
//
// Arguments:
// jumpKind - The jump kind of the new block
Expand All @@ -6734,25 +6734,43 @@ BasicBlock* Compiler::fgNewBBinRegionWorker(BBKinds jumpKind,
// Returns:
// The new block
//
// Notes:
// newBlock will be in the try region specified by tryIndex, which may not necessarily
// be the same as oldTryLast->getTryIndex() if the latter is a child region.
// However, newBlock and oldTryLast will be in the same handler region.
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we not just always do the handler fixup?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have a funclet section, and the try region we're inserting into is in a handler region, then it makes sense to always extend the handler region too. But if we don't have funclets, and we have a try region that contains a handler region ending on the old last try block, do we want to extend the handler region to cover the new last try block? The previous block check at the fgNewBBatTryRegionEnd is meant to fix the former scenario, but I suppose it's also (inadvertently) covering the second scenario too, and it hasn't given us problems yet. I'll try your suggestion and consolidate the new helper into this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without thinking about it too much, and without enough context... why are all these helpers better than the existing fgNewBBinRegion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fgNewBBinRegion does a lot of extra work to try to insert the new block in the correct region without breaking up any existing fallthrough -- most of this complexity is hidden in fgFindInsertPoint. Ideally, we wouldn't worry about block ordering at all when creating new blocks, and deferring all ordering to block layout wouldn't interfere with intermediate phases. My goal of adding this new helper is to eventually phase fgNewBBinRegion out with it such that all block creation logic is ordering-agnostic, and refactor any phases that prematurely rely on block ordering.

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();
newBlock->copyHndIndex(oldTryLast);

// Update this try region's (and all parent try regions') last block pointer
//
for (unsigned XTnum = tryIndex; XTnum < compHndBBtabCount; XTnum++)
for (unsigned XTnum = tryIndex; (XTnum < compHndBBtabCount) && (HBtab->ebdTryLast == oldTryLast); XTnum++, HBtab++)
{
assert((XTnum == tryIndex) || (XTnum == ehGetEnclosingTryIndex(XTnum - 1)));
fgSetTryEnd(HBtab, newBlock);
}

// If we inserted newBlock at the end of a handler region, repeat the above pass for handler regions
//
if (newBlock->hasHndIndex())
{
EHblkDsc* const HBtab = ehGetDsc(XTnum);
if (HBtab->ebdTryLast == oldTryLast)
const unsigned hndIndex = newBlock->getHndIndex();
HBtab = ehGetDsc(hndIndex);
for (unsigned XTnum = hndIndex; (XTnum < compHndBBtabCount) && (HBtab->ebdHndLast == oldTryLast);
XTnum++, HBtab++)
{
assert((XTnum == tryIndex) || (ehGetDsc(tryIndex)->ebdEnclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX));
fgSetTryEnd(HBtab, newBlock);
assert((XTnum == hndIndex) || (XTnum == ehGetEnclosingHndIndex(XTnum - 1)));
fgSetHndEnd(HBtab, newBlock);
}
}

assert(newBlock->getTryIndex() == tryIndex);
assert(BasicBlock::sameHndRegion(newBlock, oldTryLast));
return newBlock;
}

Expand Down
Loading