From 8cc3cffe9dac2436abca6fc0fc6f99840fdc6ec6 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 14 Nov 2023 14:24:58 +0100 Subject: [PATCH 1/2] JIT: Optimize data flow used in assertion prop/CSE The data flow used by assertion prop and CSE utilize "all-succs" which includes handler successors. However, in reality neither analysis needs full treatment of handlers; we do not CSE variables that are live-in to handlers, and assertion prop never kills assertions that would need to be propagated to handlers. Additionally, the data flow framework used conflates end-of-block propagation of facts with reachability of the handler. If no facts changed at the ends of the preds of the handler, then the handler is not visited. This is despite the fact that the handler in the general case is also reachable with facts from the beginning of every enclosed basic block (or more generally with facts that were invalidated in the middle of enclosed basic blocks). This ends up working out today only because of the fake successor edges we have from preds of the try to the handler, in addition to the restrictions on the data flows described above. Since I'm removing these fake edges, we need a more explicit solution. The change here has such a solution, by making it more explicit what exactly is needed in the data flow for CSE and AP here: they only need to consider regular successors, with the added catch that they need to consider reachability of handlers once we see the corresponding 'try' being reachable. --- src/coreclr/jit/assertionprop.cpp | 14 ++++++++----- src/coreclr/jit/dataflow.h | 34 ++++++++++++++++++++++++++++--- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 30e2db23ecd063..bd1c08fd0e76c6 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -5488,8 +5488,15 @@ class AssertionPropFlowCallback // lastTryBlock - the last block of the try for "block" handler;. // // Notes: - // We can jump to the handler from any instruction in the try region. - // It means we can propagate only assertions that are valid for the whole try region. + // We can jump to the handler from any instruction in the try region. It + // means we can propagate only assertions that are valid for the whole + // try region. + // + // It suffices to intersect with only the head 'try' block's assertions, + // since that block dominates all other blocks in the try and we never + // kill assertions in global AP. Note that if we did kill assertions we + // would need to be more careful about our mid-block handling when in a + // try region. void MergeHandler(BasicBlock* block, BasicBlock* firstTryBlock, BasicBlock* lastTryBlock) { if (VerboseDataflow()) @@ -5498,11 +5505,8 @@ class AssertionPropFlowCallback Compiler::optDumpAssertionIndices("in -> ", block->bbAssertionIn, "; "); JITDUMP("firstTryBlock " FMT_BB " ", firstTryBlock->bbNum); Compiler::optDumpAssertionIndices("in -> ", firstTryBlock->bbAssertionIn, "; "); - JITDUMP("lastTryBlock " FMT_BB " ", lastTryBlock->bbNum); - Compiler::optDumpAssertionIndices("out -> ", lastTryBlock->bbAssertionOut, "\n"); } BitVecOps::IntersectionD(apTraits, block->bbAssertionIn, firstTryBlock->bbAssertionIn); - BitVecOps::IntersectionD(apTraits, block->bbAssertionIn, lastTryBlock->bbAssertionOut); } // At the end of the merge store results of the dataflow equations, in a postmerge state. diff --git a/src/coreclr/jit/dataflow.h b/src/coreclr/jit/dataflow.h index 7601b332f5b873..c1e7afdf23b5d2 100644 --- a/src/coreclr/jit/dataflow.h +++ b/src/coreclr/jit/dataflow.h @@ -58,8 +58,7 @@ void DataFlow::ForwardAnalysis(TCallback& callback) } else { - FlowEdge* preds = m_pCompiler->BlockPredsWithEH(block); - for (FlowEdge* pred = preds; pred; pred = pred->getNextPredEdge()) + for (FlowEdge* pred = block->bbPreds; pred; pred = pred->getNextPredEdge()) { callback.Merge(block, pred->getSourceBlock(), pred->getDupCount()); } @@ -67,10 +66,39 @@ void DataFlow::ForwardAnalysis(TCallback& callback) if (callback.EndMerge(block)) { - block->VisitAllSuccs(m_pCompiler, [&worklist](BasicBlock* succ) { + // The clients using DataFlow (CSE, assertion prop) currently do + // not need EH successors here: + // + // 1. CSE does not CSE into handlers, so it considers no + // expressions available at the beginning of handlers; + // + // 2. Assertion prop never kills any assertions, so it is + // sufficient to propagate facts from the 'try' head block, since + // that block dominates all other blocks in the 'try'. That happens + // implicitly below. + // + block->VisitRegularSuccs(m_pCompiler, [&worklist](BasicBlock* succ) { worklist.insert(worklist.end(), succ); return BasicBlockVisit::Continue; }); } + + if (m_pCompiler->bbIsTryBeg(block)) + { + // Handlers of the try are reachable (and may require special + // handling compared to the normal "at-the-end" propagation above). + EHblkDsc* eh = m_pCompiler->ehGetDsc(block->getTryIndex()); + do + { + worklist.insert(worklist.end(), eh->ExFlowBlock()); + + if (eh->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) + { + break; + } + + eh = m_pCompiler->ehGetDsc(eh->ebdEnclosingTryIndex); + } while (eh->ebdTryBeg == block); + } } } From c7bd91de57242e34f449b4f42b11dbf35462ba8b Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 15 Nov 2023 11:43:06 +0100 Subject: [PATCH 2/2] Clean up, reword --- src/coreclr/jit/assertionprop.cpp | 7 +++---- src/coreclr/jit/dataflow.h | 11 ++++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index bd1c08fd0e76c6..1ca956ff70ddb8 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -5493,10 +5493,9 @@ class AssertionPropFlowCallback // try region. // // It suffices to intersect with only the head 'try' block's assertions, - // since that block dominates all other blocks in the try and we never - // kill assertions in global AP. Note that if we did kill assertions we - // would need to be more careful about our mid-block handling when in a - // try region. + // since that block dominates all other blocks in the try, and since + // assertions are VN-based and can never become false. + // void MergeHandler(BasicBlock* block, BasicBlock* firstTryBlock, BasicBlock* lastTryBlock) { if (VerboseDataflow()) diff --git a/src/coreclr/jit/dataflow.h b/src/coreclr/jit/dataflow.h index c1e7afdf23b5d2..6f27c6a998d771 100644 --- a/src/coreclr/jit/dataflow.h +++ b/src/coreclr/jit/dataflow.h @@ -58,7 +58,7 @@ void DataFlow::ForwardAnalysis(TCallback& callback) } else { - for (FlowEdge* pred = block->bbPreds; pred; pred = pred->getNextPredEdge()) + for (FlowEdge* pred : block->PredEdges()) { callback.Merge(block, pred->getSourceBlock(), pred->getDupCount()); } @@ -72,10 +72,11 @@ void DataFlow::ForwardAnalysis(TCallback& callback) // 1. CSE does not CSE into handlers, so it considers no // expressions available at the beginning of handlers; // - // 2. Assertion prop never kills any assertions, so it is - // sufficient to propagate facts from the 'try' head block, since - // that block dominates all other blocks in the 'try'. That happens - // implicitly below. + // 2. Facts in global assertion prop are VN-based and can only + // become false because of control flow, so it is sufficient to + // propagate facts available into the 'try' head block, since that + // block dominates all other blocks in the 'try'. That will happen + // as part of processing handlers below. // block->VisitRegularSuccs(m_pCompiler, [&worklist](BasicBlock* succ) { worklist.insert(worklist.end(), succ);