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

Show file tree
Hide file tree
Changes from 3 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
4 changes: 4 additions & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 handler region containing 'oldHndLast',
// as well as the end pointers of any parent handler regions, to 'newHndLast'.
void ehUpdateLastHndBlocks(BasicBlock* oldHndLast, BasicBlock* newHndLast);

// 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
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/fgbasic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6734,6 +6734,10 @@ BasicBlock* Compiler::fgNewBBinRegionWorker(BBKinds jumpKind,
// Returns:
// The new block
//
// Notes:
// It is the responsibility of the caller to set the new block's handler index,
// if it is being inserted into a handler region, too.
//
Copy link
Member

@AndyAyersMS AndyAyersMS Sep 10, 2024

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
Contributor Author

@amanasifkhalid amanasifkhalid Sep 10, 2024

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

@BruceForstall BruceForstall Sep 10, 2024

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
Contributor Author

@amanasifkhalid amanasifkhalid Sep 10, 2024

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;
Expand Down
20 changes: 20 additions & 0 deletions src/coreclr/jit/jiteh.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,26 @@ void Compiler::ehUpdateLastBlocks(BasicBlock* oldLast, BasicBlock* newLast)
}
}

//-----------------------------------------------------------------------------
// ehUpdateLastHndBlocks: Update the end pointer of the handler region containing 'oldHndLast',
// as well as the end pointers of any parent handler regions, to 'newHndLast'.
//
// Arguments:
// oldHndLast - The previous end block of the handler region(s) to be updated
// newHndLast - The new end block
//
void Compiler::ehUpdateLastHndBlocks(BasicBlock* oldHndLast, BasicBlock* newHndLast)
{
assert(oldHndLast->hasHndIndex() && BasicBlock::sameHndRegion(oldHndLast, newHndLast));
unsigned XTnum = oldHndLast->getHndIndex();
Copy link
Member

@AndyAyersMS AndyAyersMS Sep 10, 2024

Choose a reason for hiding this comment

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

Seems like you should also assert that we do at least one update (that is, that oldHndLast is indeed the last block of its enclosing handler region).

for (EHblkDsc* HBtab = ehGetDsc(XTnum); (XTnum < compHndBBtabCount) && (HBtab->ebdHndLast == oldHndLast);
XTnum++, HBtab++)
{
assert((XTnum == oldHndLast->getHndIndex()) || (ehGetEnclosingHndIndex(XTnum - 1) == XTnum));
fgSetHndEnd(HBtab, newHndLast);
}
}

unsigned Compiler::ehGetCallFinallyRegionIndex(unsigned finallyIndex, bool* inTryRegion)
{
assert(finallyIndex != EHblkDsc::NO_ENCLOSING_INDEX);
Expand Down
8 changes: 8 additions & 0 deletions src/coreclr/jit/optimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3002,6 +3002,14 @@ bool Compiler::optCanonicalizeExit(FlowGraphNaturalLoop* loop, BasicBlock* exit)
BasicBlock* finallyBlock = exit->GetTarget();
assert(finallyBlock->hasHndIndex());
newExit = fgNewBBatTryRegionEnd(BBJ_ALWAYS, finallyBlock->getHndIndex());

assert(!newExit->IsFirst());
BasicBlock* prev = newExit->Prev();
if (prev->hasHndIndex())
{
newExit->setHndIndex(prev->getHndIndex());
ehUpdateLastHndBlocks(prev, newExit);
}
}
else
{
Expand Down