From 42c447041a88db82d351f1baa5834d57a211aa85 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 9 Sep 2024 19:02:01 -0400 Subject: [PATCH 1/4] NOMERGE dump flowgraph in CI --- src/coreclr/jit/fgdiagnostic.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index dbe1b4351b0c5f..6d8b16763fd0d4 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3051,9 +3051,10 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef assert(block->hasHndIndex() == false); } } - else // reachedFirstFunclet + else if (!block->hasHndIndex()) // reachedFirstFunclet { - assert(block->hasHndIndex() == true); + fgDispBasicBlocks(); + assert(block->hasHndIndex()); } } From e1b3622cc047a7d0bee37205ab3efebee6915fe1 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 9 Sep 2024 22:50:18 -0400 Subject: [PATCH 2/4] Set handler index of exit block, if necessary --- src/coreclr/jit/compiler.h | 4 ++++ src/coreclr/jit/fgbasic.cpp | 4 ++++ src/coreclr/jit/fgdiagnostic.cpp | 5 ++--- src/coreclr/jit/jiteh.cpp | 20 ++++++++++++++++++++ src/coreclr/jit/optimizer.cpp | 8 ++++++++ 5 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a23a650a805f25..49fc5ab3b0d85a 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -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 diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 774e456f5e761a..eb51472d72766c 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -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. +// BasicBlock* Compiler::fgNewBBatTryRegionEnd(BBKinds jumpKind, unsigned tryIndex) { BasicBlock* const oldTryLast = ehGetDsc(tryIndex)->ebdTryLast; diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 6d8b16763fd0d4..dbe1b4351b0c5f 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3051,10 +3051,9 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef assert(block->hasHndIndex() == false); } } - else if (!block->hasHndIndex()) // reachedFirstFunclet + else // reachedFirstFunclet { - fgDispBasicBlocks(); - assert(block->hasHndIndex()); + assert(block->hasHndIndex() == true); } } diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 3ba4e73353db1e..3eeb7a8e795ffc 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -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(); + 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); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index f9880026eab268..8a4217e3bd01de 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -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->IsLast()); + BasicBlock* prev = newExit->Prev(); + if (prev->hasHndIndex()) + { + newExit->setHndIndex(prev->getHndIndex()); + ehUpdateLastHndBlocks(prev, newExit); + } } else { From 26cb4e86df563a6b0219897c5c0ea4afd267a1b5 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 10 Sep 2024 09:42:56 -0400 Subject: [PATCH 3/4] Fix assert --- src/coreclr/jit/optimizer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 8a4217e3bd01de..4b05cc1dfac407 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -3003,7 +3003,7 @@ bool Compiler::optCanonicalizeExit(FlowGraphNaturalLoop* loop, BasicBlock* exit) assert(finallyBlock->hasHndIndex()); newExit = fgNewBBatTryRegionEnd(BBJ_ALWAYS, finallyBlock->getHndIndex()); - assert(!newExit->IsLast()); + assert(!newExit->IsFirst()); BasicBlock* prev = newExit->Prev(); if (prev->hasHndIndex()) { From 03ac05a16f3933a21ad0921f3e310b153b6eaf6b Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 10 Sep 2024 11:58:31 -0400 Subject: [PATCH 4/4] Update handler region ends in fgNewBBatTryRegionEnd --- src/coreclr/jit/compiler.h | 4 ---- src/coreclr/jit/fgbasic.cpp | 34 ++++++++++++++++++++++++---------- src/coreclr/jit/jiteh.cpp | 20 -------------------- src/coreclr/jit/optimizer.cpp | 8 -------- 4 files changed, 24 insertions(+), 42 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 49fc5ab3b0d85a..a23a650a805f25 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2858,10 +2858,6 @@ 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 diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index eb51472d72766c..39531ea99aa22d 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -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 @@ -6735,28 +6735,42 @@ BasicBlock* Compiler::fgNewBBinRegionWorker(BBKinds jumpKind, // 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. +// 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. // 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++) { - EHblkDsc* const HBtab = ehGetDsc(XTnum); - if (HBtab->ebdTryLast == oldTryLast) + 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()) + { + 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; } diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 3eeb7a8e795ffc..3ba4e73353db1e 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -901,26 +901,6 @@ 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(); - 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); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 4b05cc1dfac407..f9880026eab268 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -3002,14 +3002,6 @@ 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 {