From 9cc489a2887d09ea224fb74c2ec02e99d6a85e5e Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 30 Oct 2024 16:49:27 -0700 Subject: [PATCH 1/4] JIT: account for newly unreachable blocks in morph Morph can alter control flow, if (for instance) it can prove a block's conditional branch must go a certain way. Take advantage of this to improve morph's cross-block assertion prop, by not considering unreachable predecessors when computing the incoming assertion state. This is similar to something we already do in value numbering, but there control flow isn't being altered. Also, when we can prove that a block has become unreachable, remove the block IR and alter its jump kind, so we are not spending time morphing IR we'll subsequently just throw away. --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/morph.cpp | 73 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 56410dfd3035a9..538774f6559822 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5451,7 +5451,7 @@ class Compiler FoldResult fgFoldConditional(BasicBlock* block); PhaseStatus fgMorphBlocks(); - void fgMorphBlock(BasicBlock* block); + void fgMorphBlock(BasicBlock* block, BlockSet* unreachable = nullptr); void fgMorphStmts(BasicBlock* block); void fgMergeBlockReturn(BasicBlock* block); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 1e8f2d7c45ef80..1e2494de704956 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13212,11 +13212,30 @@ void Compiler::fgMorphStmts(BasicBlock* block) // // Arguments: // block - block in question +// unreachable - [optional] set of blocks proven to be unreachable // -void Compiler::fgMorphBlock(BasicBlock* block) +void Compiler::fgMorphBlock(BasicBlock* block, BlockSet* unreachable) { JITDUMP("\nMorphing " FMT_BB "\n", block->bbNum); + // Helpers for reachability tracking + // + auto SetUnreachable = [=](BasicBlock* block) { + if (unreachable != nullptr) + { + BlockSetOps::AddElemD(this, *unreachable, block->bbNum); + } + }; + + auto IsReachable = [=](BasicBlock* block) { + if (unreachable == nullptr) + { + return false; + } + assert(m_dfsTree->Contains(block)); + return !BlockSetOps::IsMember(this, *unreachable, block->bbNum); + }; + if (optLocalAssertionProp) { if (!optCrossBlockLocalAssertionProp) @@ -13243,6 +13262,8 @@ void Compiler::fgMorphBlock(BasicBlock* block) else { bool hasPredAssertions = false; + bool isReachable = + (block == fgFirstBB) || (block == genReturnBB) || (opts.IsOSR() && (block == fgEntryBB)); for (BasicBlock* const pred : block->PredBlocks()) { @@ -13257,10 +13278,27 @@ void Compiler::fgMorphBlock(BasicBlock* block) JITDUMP(FMT_BB " pred " FMT_BB " not processed; clearing assertions in\n", block->bbNum, pred->bbNum); hasPredAssertions = false; + + // Generally we must assume this pred will be reachable. + // + isReachable = true; break; } - // Yes, pred assertions are available. + // This pred was reachable in the pre-morph DFS, but might have + // become unreachable during morph. If so, we can ignore its assertion state. + // + if (!IsReachable(pred)) + { + JITDUMP("Pred " FMT_BB " is no longer reachable\n", pred->bbNum); + continue; + } + + // Since we have a reachable pred, this blocks is also reachable. + // + isReachable = true; + + // This pred is reachable and has available assertions. // If the pred is (a non-degenerate) BBJ_COND, fetch the appropriate out set. // ASSERT_TP assertionsOut; @@ -13312,6 +13350,30 @@ void Compiler::fgMorphBlock(BasicBlock* block) // canUsePredAssertions = false; } + + // If there wasn't any reachable pred, this block is also not + // reachable. Note we exclude handler entries above, since we don't + // do the correct assertion tracking for handlers. Thus there is + // no need to consider reachable EH preds. + // + if (!isReachable) + { + JITDUMP(FMT_BB " has no reachable preds, marking as unreachable\n", block->bbNum); + SetUnreachable(block); + + // Remove the block's IR and flow edges but don't mark the block as removed. + // Convert to BBJ_THROW. But leave CALLFINALLY alone. + // + // In any case, there is nothing to morph, so just return. + // + if (!block->KindIs(BBJ_CALLFINALLY)) + { + fgUnreachableBlock(block); + block->RemoveFlags(BBF_REMOVED); + block->SetKindAndTargetEdge(BBJ_THROW); + } + return; + } } if (!canUsePredAssertions) @@ -13430,6 +13492,11 @@ PhaseStatus Compiler::fgMorphBlocks() INDEBUG(fgSafeBasicBlockCreation = false;); INDEBUG(fgSafeFlowEdgeCreation = false;); + // We will track which blocks become unreachable during morph + // + EnsureBasicBlockEpoch(); + BlockSet unreachable = BlockSetOps::MakeEmpty(this); + // Allow edge creation to genReturnBB (target of return merging) // and the scratch block successor (target for tail call to loop). // This will also disallow dataflow into these blocks. @@ -13452,7 +13519,7 @@ PhaseStatus Compiler::fgMorphBlocks() for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) { BasicBlock* const block = m_dfsTree->GetPostOrder(i - 1); - fgMorphBlock(block); + fgMorphBlock(block, &unreachable); } assert(bbNumMax == fgBBNumMax); From a61b9494b2f065411d1156e6e3c443a601e24a46 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 31 Oct 2024 11:09:49 -0700 Subject: [PATCH 2/4] leave x86 callfinallys alone --- src/coreclr/jit/morph.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 1e2494de704956..e851cd55226d15 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13362,17 +13362,17 @@ void Compiler::fgMorphBlock(BasicBlock* block, BlockSet* unreachable) SetUnreachable(block); // Remove the block's IR and flow edges but don't mark the block as removed. - // Convert to BBJ_THROW. But leave CALLFINALLY alone. + // Convert to BBJ_THROW. But leave CALLFINALLY for non-funclet EH alone. // - // In any case, there is nothing to morph, so just return. + // If we clear out the block, there is nothing to morph, so just return. // - if (!block->KindIs(BBJ_CALLFINALLY)) + if (UsesFunclets() || !block->KindIs(BBJ_CALLFINALLY)) { fgUnreachableBlock(block); block->RemoveFlags(BBF_REMOVED); block->SetKindAndTargetEdge(BBJ_THROW); + return; } - return; } } From faf1af9ae071ab790bc8a466b6dce00224727086 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 31 Oct 2024 13:31:46 -0700 Subject: [PATCH 3/4] use more compact bit vectors --- src/coreclr/jit/compiler.h | 14 +++++++- src/coreclr/jit/morph.cpp | 66 +++++++++++++++++++++++--------------- 2 files changed, 54 insertions(+), 26 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 538774f6559822..b0fe932cb435eb 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5450,8 +5450,20 @@ class Compiler FoldResult fgFoldConditional(BasicBlock* block); + struct MorphUnreachableInfo + { + MorphUnreachableInfo(Compiler* comp); + void SetUnreachable(BasicBlock* block); + bool IsUnreachable(BasicBlock* block); + + private: + + BitVecTraits m_traits; + BitVec m_vec; + }; + PhaseStatus fgMorphBlocks(); - void fgMorphBlock(BasicBlock* block, BlockSet* unreachable = nullptr); + void fgMorphBlock(BasicBlock* block, MorphUnreachableInfo* unreachableInfo = nullptr); void fgMorphStmts(BasicBlock* block); void fgMergeBlockReturn(BasicBlock* block); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index e851cd55226d15..4f66e773717c10 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13208,33 +13208,50 @@ void Compiler::fgMorphStmts(BasicBlock* block) } //------------------------------------------------------------------------ -// fgMorphBlock: Morph a basic block +// MorphUnreachbleInfo: construct info for unreachability tracking during morph +// +// Arguments: +// comp - compiler object +// +Compiler::MorphUnreachableInfo::MorphUnreachableInfo(Compiler* comp) + : m_traits(comp->m_dfsTree->GetPostOrderCount(), comp) + , m_vec(BitVecOps::MakeEmpty(&m_traits)){}; + +//------------------------------------------------------------------------ +// SetUnreachable: during morph, mark a block as unreachable // // Arguments: // block - block in question -// unreachable - [optional] set of blocks proven to be unreachable // -void Compiler::fgMorphBlock(BasicBlock* block, BlockSet* unreachable) +void Compiler::MorphUnreachableInfo::SetUnreachable(BasicBlock* block) { - JITDUMP("\nMorphing " FMT_BB "\n", block->bbNum); + BitVecOps::AddElemD(&m_traits, m_vec, block->bbPostorderNum); +} - // Helpers for reachability tracking - // - auto SetUnreachable = [=](BasicBlock* block) { - if (unreachable != nullptr) - { - BlockSetOps::AddElemD(this, *unreachable, block->bbNum); - } - }; +//------------------------------------------------------------------------ +// IsUnreachable: during morph, see if a block is now known to be unreachable +// +// Arguments: +// block - block in question +// +// Returns: +// true if so +// +bool Compiler::MorphUnreachableInfo::IsUnreachable(BasicBlock* block) +{ + return BitVecOps::IsMember(&m_traits, m_vec, block->bbPostorderNum); +} - auto IsReachable = [=](BasicBlock* block) { - if (unreachable == nullptr) - { - return false; - } - assert(m_dfsTree->Contains(block)); - return !BlockSetOps::IsMember(this, *unreachable, block->bbNum); - }; +//------------------------------------------------------------------------ +// fgMorphBlock: Morph a basic block +// +// Arguments: +// block - block in question +// unreachableInfo - [optional] info on blocks proven unreachable +// +void Compiler::fgMorphBlock(BasicBlock* block, MorphUnreachableInfo* unreachableInfo) +{ + JITDUMP("\nMorphing " FMT_BB "\n", block->bbNum); if (optLocalAssertionProp) { @@ -13288,7 +13305,7 @@ void Compiler::fgMorphBlock(BasicBlock* block, BlockSet* unreachable) // This pred was reachable in the pre-morph DFS, but might have // become unreachable during morph. If so, we can ignore its assertion state. // - if (!IsReachable(pred)) + if (unreachableInfo->IsUnreachable(pred)) { JITDUMP("Pred " FMT_BB " is no longer reachable\n", pred->bbNum); continue; @@ -13359,7 +13376,7 @@ void Compiler::fgMorphBlock(BasicBlock* block, BlockSet* unreachable) if (!isReachable) { JITDUMP(FMT_BB " has no reachable preds, marking as unreachable\n", block->bbNum); - SetUnreachable(block); + unreachableInfo->SetUnreachable(block); // Remove the block's IR and flow edges but don't mark the block as removed. // Convert to BBJ_THROW. But leave CALLFINALLY for non-funclet EH alone. @@ -13494,8 +13511,7 @@ PhaseStatus Compiler::fgMorphBlocks() // We will track which blocks become unreachable during morph // - EnsureBasicBlockEpoch(); - BlockSet unreachable = BlockSetOps::MakeEmpty(this); + MorphUnreachableInfo unreachableInfo(this); // Allow edge creation to genReturnBB (target of return merging) // and the scratch block successor (target for tail call to loop). @@ -13519,7 +13535,7 @@ PhaseStatus Compiler::fgMorphBlocks() for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) { BasicBlock* const block = m_dfsTree->GetPostOrder(i - 1); - fgMorphBlock(block, &unreachable); + fgMorphBlock(block, &unreachableInfo); } assert(bbNumMax == fgBBNumMax); From 42f676cb3b3b36c5b337957d649192c5fdec1af9 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 31 Oct 2024 15:50:57 -0700 Subject: [PATCH 4/4] callfinally, you win --- src/coreclr/jit/morph.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 4f66e773717c10..252dd0bedfecca 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13379,11 +13379,12 @@ void Compiler::fgMorphBlock(BasicBlock* block, MorphUnreachableInfo* unreachable unreachableInfo->SetUnreachable(block); // Remove the block's IR and flow edges but don't mark the block as removed. - // Convert to BBJ_THROW. But leave CALLFINALLY for non-funclet EH alone. + // Convert to BBJ_THROW. But leave CALLFINALLY alone. // // If we clear out the block, there is nothing to morph, so just return. // - if (UsesFunclets() || !block->KindIs(BBJ_CALLFINALLY)) + bool const isCallFinally = block->KindIs(BBJ_CALLFINALLY); + if (!isCallFinally) { fgUnreachableBlock(block); block->RemoveFlags(BBF_REMOVED);