From c6006c613ead3e57383a8203e319ba0348ad107a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 10 Jan 2024 22:10:39 +0100 Subject: [PATCH] JIT: Canonicalize newly recognized loops (#96751) Create preheaders for all loops, not just loops recognized by old loop finding. Requires adding support for creating preheaders when the loop header is the beginning of a handler block, which requires updating the EH table (so we switch to fgExtendEHRegionBefore). --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgdiagnostic.cpp | 6 --- src/coreclr/jit/optimizer.cpp | 93 ++++++++++++++------------------ 3 files changed, 42 insertions(+), 59 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index abeb6c728c8da7..adb18d25119266 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5981,7 +5981,7 @@ class Compiler PhaseStatus fgCanonicalizeFirstBB(); - void fgSetEHRegionForNewLoopHead(BasicBlock* newHead, BasicBlock* top); + void fgSetEHRegionForNewPreheader(BasicBlock* preheader); void fgUnreachableBlock(BasicBlock* block); diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 7ff4cd317b6798..d0c0bb39e04633 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -4787,12 +4787,6 @@ void Compiler::fgDebugCheckLoopTable() { for (FlowGraphNaturalLoop* loop : m_loops->InReversePostOrder()) { - // TODO-Quirk: Remove - if (!loop->GetHeader()->HasFlag(BBF_OLD_LOOP_HEADER_QUIRK)) - { - continue; - } - assert(loop->EntryEdges().size() == 1); assert(loop->EntryEdge(0)->getSourceBlock()->KindIs(BBJ_ALWAYS)); } diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 598193a26b7827..59703b2297c42d 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -4927,12 +4927,6 @@ bool Compiler::optCanonicalizeLoops(FlowGraphNaturalLoops* loops) bool changed = false; for (FlowGraphNaturalLoop* loop : loops->InReversePostOrder()) { - // TODO-Quirk: Remove - if (!loop->GetHeader()->HasFlag(BBF_OLD_LOOP_HEADER_QUIRK)) - { - continue; - } - changed |= optCreatePreheader(loop); } @@ -4986,7 +4980,7 @@ bool Compiler::optCreatePreheader(FlowGraphNaturalLoop* loop) BasicBlock* preheader = fgNewBBbefore(BBJ_ALWAYS, insertBefore, false, header); preheader->SetFlags(BBF_INTERNAL | BBF_LOOP_PREHEADER); - fgSetEHRegionForNewLoopHead(preheader, insertBefore); + fgSetEHRegionForNewPreheader(preheader); if (preheader->NextIs(header)) { @@ -5138,7 +5132,7 @@ void Compiler::optSetPreheaderWeight(FlowGraphNaturalLoop* loop, BasicBlock* pre fgHaveValidEdgeWeights ? (useEdgeWeights ? "valid" : "ignored") : "invalid", loopEnteredCount, loopSkippedCount, loopTakenRatio); - // Calculate a good approximation of the preHead's block weight + // Calculate a good approximation of the preheader's block weight weight_t preheaderWeight = (prevEntering->bbWeight * loopTakenRatio); preheader->setBBProfileWeight(preheaderWeight); assert(!preheader->isRunRarely()); @@ -7427,70 +7421,65 @@ bool Compiler::optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNS } //------------------------------------------------------------------------------ -// fgSetEHRegionForNewLoopHead: When a new loop HEAD block is created, this sets the EH region properly for -// the new block. +// fgSetEHRegionForNewPreheader: Set the EH region for a newly inserted +// preheader. // // In which EH region should the header live? // -// The header block is added immediately before `top`. -// -// The `top` block cannot be the first block of a filter or handler: `top` must have a back-edge from a -// BBJ_COND or BBJ_ALWAYS within the loop, and a filter or handler cannot be branched to like that. -// -// The `top` block can be the first block of a `try` region, and you can fall into or branch to the -// first block of a `try` region. (For top-entry loops, `top` will both be the target of a back-edge -// and a fall-through from the previous block.) -// -// If the `top` block is NOT the first block of a `try` region, the header can simply extend the -// `top` block region. -// -// If the `top` block IS the first block of a `try`, we find its parent region and use that. For mutual-protect -// regions, we need to find the actual parent, as the block stores the most "nested" mutual region. For -// non-mutual-protect regions, due to EH canonicalization, we are guaranteed that no other EH regions begin -// on the same block, so looking to just the parent is sufficient. Note that we can't just extend the EH -// region of `top` to the header, because `top` will still be the target of backward branches from -// within the loop. If those backward branches come from outside the `try` (say, only the top half of the loop -// is a `try` region), then we can't branch to a non-first `try` region block (you always must entry the `try` -// in the first block). -// -// Note that hoisting any code out of a try region, for example, to a pre-header block in a different -// EH region, needs to ensure that no exceptions will be thrown. +// The preheader block is expected to have been added immediately before a +// block `next` in the loop that is also in the same EH region as the header. +// This is usually the lexically first block of the loop, but may also be the +// header itself. +// +// If the `next` block is NOT the first block of a `try` region, the preheader +// can simply extend the header block's EH region. +// +// If the `next` block IS the first block of a `try`, we find its parent region +// and use that. For mutual-protect regions, we need to find the actual parent, +// as the block stores the most "nested" mutual region. For non-mutual-protect +// regions, due to EH canonicalization, we are guaranteed that no other EH +// regions begin on the same block, so looking to just the parent is +// sufficient. Note that we can't just extend the EH region of the header to +// the preheader, because the header will still be the target of backward +// branches from within the loop. If those backward branches come from outside +// the `try` (say, only the top half of the loop is a `try` region), then we +// can't branch to a non-first `try` region block (you always must enter the +// `try` in the first block). +// +// Note that hoisting any code out of a try region, for example, to a preheader +// block in a different EH region, needs to ensure that no exceptions will be +// thrown. // // Arguments: -// newHead - the new `head` block, which has already been added to the block list ahead of the loop `top` -// top - the loop `top` block +// preheader - the new preheader block, which has already been added to the +// block list before a block inside the loop that shares EH +// region with the header. // -void Compiler::fgSetEHRegionForNewLoopHead(BasicBlock* newHead, BasicBlock* top) +void Compiler::fgSetEHRegionForNewPreheader(BasicBlock* preheader) { - assert(newHead->NextIs(top)); - assert(!fgIsFirstBlockOfFilterOrHandler(top)); + BasicBlock* next = preheader->Next(); - if (bbIsTryBeg(top)) + if (bbIsTryBeg(next)) { - // `top` is the beginning of a try block. Figure out the EH region to use. - assert(top->hasTryIndex()); - unsigned newTryIndex = ehTrueEnclosingTryIndexIL(top->getTryIndex()); + // `next` is the beginning of a try block. Figure out the EH region to use. + assert(next->hasTryIndex()); + unsigned newTryIndex = ehTrueEnclosingTryIndexIL(next->getTryIndex()); if (newTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) { // No EH try index. - newHead->clearTryIndex(); + preheader->clearTryIndex(); } else { - newHead->setTryIndex(newTryIndex); + preheader->setTryIndex(newTryIndex); } - // What handler region to use? Use the same handler region as `top`. - newHead->copyHndIndex(top); + // What handler region to use? Use the same handler region as `next`. + preheader->copyHndIndex(next); } else { - // `top` is not the beginning of a try block. Just extend the EH region to the pre-header. - // We don't need to call `fgExtendEHRegionBefore()` because all the special handling that function - // does it to account for `top` being the first block of a `try` or handler region, which we know - // is not true. - - newHead->copyEHRegion(top); + fgExtendEHRegionBefore(next); } }