From 37df2cfbf804698b378c685a937d20bc4f809155 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Mon, 19 Jan 2015 12:09:11 +0000 Subject: [PATCH] [PM] Remove the Pass argument from all of the critical edge splitting APIs and replace it and numerous booleans with an option struct. The critical edge splitting API has a really large surface of flags and so it seems worth burning a small option struct / builder. This struct can be constructed with the various preserved analyses and then flags can be flipped in a builder style. The various users are now responsible for directly passing along their analysis information. This should be enough for the critical edge splitting to work cleanly with the new pass manager as well. This API is still pretty crufty and could be cleaned up a lot, but I've focused on this change just threading an option struct rather than a pass through the API. llvm-svn: 226456 --- .../llvm/Transforms/Utils/BasicBlockUtils.h | 116 +++++++++++++----- .../CodeGen/SelectionDAG/SelectionDAGISel.cpp | 9 +- .../Instrumentation/SanitizerCoverage.cpp | 2 +- llvm/lib/Transforms/Scalar/GVN.cpp | 6 +- llvm/lib/Transforms/Scalar/LoopRotation.cpp | 7 +- .../Transforms/Scalar/LoopStrengthReduce.cpp | 7 +- llvm/lib/Transforms/Scalar/LoopUnswitch.cpp | 7 +- llvm/lib/Transforms/Utils/BasicBlockUtils.cpp | 21 ++-- .../Transforms/Utils/BreakCriticalEdges.cpp | 41 +++---- 9 files changed, 141 insertions(+), 75 deletions(-) diff --git a/llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h b/llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h index da1988c3f2d6..6a35aae01e32 100644 --- a/llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h +++ b/llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h @@ -76,18 +76,71 @@ void ReplaceInstWithInst(BasicBlock::InstListType &BIL, // void ReplaceInstWithInst(Instruction *From, Instruction *To); -/// SplitCriticalEdge - If this edge is a critical edge, insert a new node to -/// split the critical edge. This will update DominatorTree and -/// DominatorFrontier information if it is available, thus calling this pass -/// will not invalidate either of them. This returns the new block if the edge -/// was split, null otherwise. +/// \brief Option class for critical edge splitting. /// -/// If MergeIdenticalEdges is true (not the default), *all* edges from TI to the -/// specified successor will be merged into the same critical edge block. -/// This is most commonly interesting with switch instructions, which may -/// have many edges to any one destination. This ensures that all edges to that -/// dest go to one block instead of each going to a different block, but isn't -/// the standard definition of a "critical edge". +/// This provides a builder interface for overriding the default options used +/// during critical edge splitting. +struct CriticalEdgeSplittingOptions { + AliasAnalysis *AA; + DominatorTree *DT; + LoopInfo *LI; + bool MergeIdenticalEdges; + bool DontDeleteUselessPHIs; + bool SplitLandingPads; + bool PreserveLCSSA; + + CriticalEdgeSplittingOptions() + : AA(nullptr), DT(nullptr), LI(nullptr), MergeIdenticalEdges(false), + DontDeleteUselessPHIs(false), SplitLandingPads(false), + PreserveLCSSA(false) {} + + /// \brief Basic case of setting up all the analysis. + CriticalEdgeSplittingOptions(AliasAnalysis *AA, DominatorTree *DT = nullptr, + LoopInfo *LI = nullptr) + : AA(AA), DT(DT), LI(LI), MergeIdenticalEdges(false), + DontDeleteUselessPHIs(false), SplitLandingPads(false), + PreserveLCSSA(false) {} + + /// \brief A common pattern is to preserve the dominator tree and loop + /// info but not care about AA. + CriticalEdgeSplittingOptions(DominatorTree *DT, LoopInfo *LI) + : AA(nullptr), DT(DT), LI(LI), MergeIdenticalEdges(false), + DontDeleteUselessPHIs(false), SplitLandingPads(false), + PreserveLCSSA(false) {} + + CriticalEdgeSplittingOptions &setMergeIdenticalEdges() { + MergeIdenticalEdges = true; + return *this; + } + + CriticalEdgeSplittingOptions &setDontDeleteUselessPHIs() { + DontDeleteUselessPHIs = true; + return *this; + } + + CriticalEdgeSplittingOptions &setSplitLandingPads() { + SplitLandingPads = true; + return *this; + } + + CriticalEdgeSplittingOptions &setPreserveLCSSA() { + PreserveLCSSA = true; + return *this; + } +}; + +/// SplitCriticalEdge - If this edge is a critical edge, insert a new node to +/// split the critical edge. This will update the analyses passed in through +/// the option struct. This returns the new block if the edge was split, null +/// otherwise. +/// +/// If MergeIdenticalEdges in the options struct is true (not the default), +/// *all* edges from TI to the specified successor will be merged into the same +/// critical edge block. This is most commonly interesting with switch +/// instructions, which may have many edges to any one destination. This +/// ensures that all edges to that dest go to one block instead of each going +/// to a different block, but isn't the standard definition of a "critical +/// edge". /// /// It is invalid to call this function on a critical edge that starts at an /// IndirectBrInst. Splitting these edges will almost always create an invalid @@ -95,14 +148,15 @@ void ReplaceInstWithInst(Instruction *From, Instruction *To); /// to. /// BasicBlock *SplitCriticalEdge(TerminatorInst *TI, unsigned SuccNum, - Pass *P = nullptr, - bool MergeIdenticalEdges = false, - bool DontDeleteUselessPHIs = false, - bool SplitLandingPads = false); + const CriticalEdgeSplittingOptions &Options = + CriticalEdgeSplittingOptions()); -inline BasicBlock *SplitCriticalEdge(BasicBlock *BB, succ_iterator SI, - Pass *P = nullptr) { - return SplitCriticalEdge(BB->getTerminator(), SI.getSuccessorIndex(), P); +inline BasicBlock * +SplitCriticalEdge(BasicBlock *BB, succ_iterator SI, + const CriticalEdgeSplittingOptions &Options = + CriticalEdgeSplittingOptions()) { + return SplitCriticalEdge(BB->getTerminator(), SI.getSuccessorIndex(), + Options); } /// SplitCriticalEdge - If the edge from *PI to BB is not critical, return @@ -111,38 +165,40 @@ inline BasicBlock *SplitCriticalEdge(BasicBlock *BB, succ_iterator SI, /// function. If P is specified, it updates the analyses /// described above. inline bool SplitCriticalEdge(BasicBlock *Succ, pred_iterator PI, - Pass *P = nullptr) { + const CriticalEdgeSplittingOptions &Options = + CriticalEdgeSplittingOptions()) { bool MadeChange = false; TerminatorInst *TI = (*PI)->getTerminator(); for (unsigned i = 0, e = TI->getNumSuccessors(); i != e; ++i) if (TI->getSuccessor(i) == Succ) - MadeChange |= !!SplitCriticalEdge(TI, i, P); + MadeChange |= !!SplitCriticalEdge(TI, i, Options); return MadeChange; } /// SplitCriticalEdge - If an edge from Src to Dst is critical, split the edge /// and return true, otherwise return false. This method requires that there be -/// an edge between the two blocks. If P is specified, it updates the analyses -/// described above. -inline BasicBlock *SplitCriticalEdge(BasicBlock *Src, BasicBlock *Dst, - Pass *P = nullptr, - bool MergeIdenticalEdges = false, - bool DontDeleteUselessPHIs = false) { +/// an edge between the two blocks. It updates the analyses +/// passed in the options struct +inline BasicBlock * +SplitCriticalEdge(BasicBlock *Src, BasicBlock *Dst, + const CriticalEdgeSplittingOptions &Options = + CriticalEdgeSplittingOptions()) { TerminatorInst *TI = Src->getTerminator(); unsigned i = 0; while (1) { assert(i != TI->getNumSuccessors() && "Edge doesn't exist!"); if (TI->getSuccessor(i) == Dst) - return SplitCriticalEdge(TI, i, P, MergeIdenticalEdges, - DontDeleteUselessPHIs); + return SplitCriticalEdge(TI, i, Options); ++i; } } // SplitAllCriticalEdges - Loop over all of the edges in the CFG, -// breaking critical edges as they are found. Pass P must not be NULL. +// breaking critical edges as they are found. // Returns the number of broken edges. -unsigned SplitAllCriticalEdges(Function &F, Pass *P); +unsigned SplitAllCriticalEdges(Function &F, + const CriticalEdgeSplittingOptions &Options = + CriticalEdgeSplittingOptions()); /// SplitEdge - Split the edge connecting specified block. Pass P must /// not be NULL. diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp index 6a0a07970a80..d37de980e067 100644 --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp @@ -379,7 +379,7 @@ void SelectionDAGISel::getAnalysisUsage(AnalysisUsage &AU) const { /// /// This is required for correctness, so it must be done at -O0. /// -static void SplitCriticalSideEffectEdges(Function &Fn, Pass *SDISel) { +static void SplitCriticalSideEffectEdges(Function &Fn, AliasAnalysis *AA) { // Loop for blocks with phi nodes. for (Function::iterator BB = Fn.begin(), E = Fn.end(); BB != E; ++BB) { PHINode *PN = dyn_cast(BB->begin()); @@ -403,8 +403,9 @@ static void SplitCriticalSideEffectEdges(Function &Fn, Pass *SDISel) { continue; // Okay, we have to split this edge. - SplitCriticalEdge(Pred->getTerminator(), - GetSuccessorNumber(Pred, BB), SDISel, true); + SplitCriticalEdge( + Pred->getTerminator(), GetSuccessorNumber(Pred, BB), + CriticalEdgeSplittingOptions(AA).setMergeIdenticalEdges()); goto ReprocessBlock; } } @@ -441,7 +442,7 @@ bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) { DEBUG(dbgs() << "\n\n\n=== " << Fn.getName() << "\n"); - SplitCriticalSideEffectEdges(const_cast(Fn), this); + SplitCriticalSideEffectEdges(const_cast(Fn), AA); CurDAG->init(*MF); FuncInfo->set(Fn, *MF, CurDAG); diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp index c048a99f8880..b221ef8f7e6e 100644 --- a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp +++ b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp @@ -199,7 +199,7 @@ bool SanitizerCoverageModule::runOnFunction(Function &F) { if (F.getName().find(".module_ctor") != std::string::npos) return false; // Should not instrument sanitizer init functions. if (CoverageLevel >= 3) - SplitAllCriticalEdges(F, this); + SplitAllCriticalEdges(F); SmallVector IndirCalls; SmallVector AllBlocks; for (auto &BB : F) { diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp index 86bee09080b0..29ea432e4a4c 100644 --- a/llvm/lib/Transforms/Scalar/GVN.cpp +++ b/llvm/lib/Transforms/Scalar/GVN.cpp @@ -2635,7 +2635,8 @@ bool GVN::performPRE(Function &F) { /// Split the critical edge connecting the given two blocks, and return /// the block inserted to the critical edge. BasicBlock *GVN::splitCriticalEdges(BasicBlock *Pred, BasicBlock *Succ) { - BasicBlock *BB = SplitCriticalEdge(Pred, Succ, this); + BasicBlock *BB = SplitCriticalEdge( + Pred, Succ, CriticalEdgeSplittingOptions(getAliasAnalysis(), DT)); if (MD) MD->invalidateCachedPredecessors(); return BB; @@ -2648,7 +2649,8 @@ bool GVN::splitCriticalEdges() { return false; do { std::pair Edge = toSplit.pop_back_val(); - SplitCriticalEdge(Edge.first, Edge.second, this); + SplitCriticalEdge(Edge.first, Edge.second, + CriticalEdgeSplittingOptions(getAliasAnalysis(), DT)); } while (!toSplit.empty()); if (MD) MD->invalidateCachedPredecessors(); return true; diff --git a/llvm/lib/Transforms/Scalar/LoopRotation.cpp b/llvm/lib/Transforms/Scalar/LoopRotation.cpp index c11510ffd083..c37cd917ad2a 100644 --- a/llvm/lib/Transforms/Scalar/LoopRotation.cpp +++ b/llvm/lib/Transforms/Scalar/LoopRotation.cpp @@ -519,7 +519,9 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) { // Right now OrigPreHeader has two successors, NewHeader and ExitBlock, and // thus is not a preheader anymore. // Split the edge to form a real preheader. - BasicBlock *NewPH = SplitCriticalEdge(OrigPreheader, NewHeader, this); + BasicBlock *NewPH = SplitCriticalEdge( + OrigPreheader, NewHeader, + CriticalEdgeSplittingOptions(DT, LI).setPreserveLCSSA()); NewPH->setName(NewHeader->getName() + ".lr.ph"); // Preserve canonical loop form, which means that 'Exit' should have only @@ -536,7 +538,8 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) { if (!PredLoop || PredLoop->contains(Exit)) continue; SplitLatchEdge |= L->getLoopLatch() == *PI; - BasicBlock *ExitSplit = SplitCriticalEdge(*PI, Exit, this); + BasicBlock *ExitSplit = SplitCriticalEdge( + *PI, Exit, CriticalEdgeSplittingOptions(DT, LI).setPreserveLCSSA()); ExitSplit->moveBefore(Exit); } assert(SplitLatchEdge && diff --git a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp index 629b0a2b3b00..fdd4ff547b44 100644 --- a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp +++ b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp @@ -4728,9 +4728,10 @@ void LSRInstance::RewriteForPHI(PHINode *PN, // Split the critical edge. BasicBlock *NewBB = nullptr; if (!Parent->isLandingPad()) { - NewBB = SplitCriticalEdge(BB, Parent, P, - /*MergeIdenticalEdges=*/true, - /*DontDeleteUselessPhis=*/true); + NewBB = SplitCriticalEdge(BB, Parent, + CriticalEdgeSplittingOptions(&DT, &LI) + .setMergeIdenticalEdges() + .setDontDeleteUselessPHIs()); } else { SmallVector NewBBs; SplitLandingPadPredecessors(Parent, BB, "", "", NewBBs, diff --git a/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp index 30d6d510acef..e809a5ccc84a 100644 --- a/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp +++ b/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp @@ -706,8 +706,11 @@ void LoopUnswitch::EmitPreheaderBranchOnCondition(Value *LIC, Constant *Val, // If either edge is critical, split it. This helps preserve LoopSimplify // form for enclosing loops. - SplitCriticalEdge(BI, 0, this, false, false, true); - SplitCriticalEdge(BI, 1, this, false, false, true); + auto Options = CriticalEdgeSplittingOptions(DT, LI) + .setPreserveLCSSA() + .setSplitLandingPads(); + SplitCriticalEdge(BI, 0, Options); + SplitCriticalEdge(BI, 1, Options); } /// UnswitchTrivialCondition - Given a loop that has a trivial unswitchable diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp index c2304d7642bf..866fb83b140b 100644 --- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp +++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp @@ -234,15 +234,20 @@ void llvm::ReplaceInstWithInst(Instruction *From, Instruction *To) { BasicBlock *llvm::SplitEdge(BasicBlock *BB, BasicBlock *Succ, Pass *P) { unsigned SuccNum = GetSuccessorNumber(BB, Succ); - // If this is a critical edge, let SplitCriticalEdge do it. - TerminatorInst *LatchTerm = BB->getTerminator(); - if (SplitCriticalEdge(LatchTerm, SuccNum, P)) - return LatchTerm->getSuccessor(SuccNum); - + auto *AA = P->getAnalysisIfAvailable(); auto *DTWP = P->getAnalysisIfAvailable(); auto *DT = DTWP ? &DTWP->getDomTree() : nullptr; auto *LIWP = P->getAnalysisIfAvailable(); auto *LI = LIWP ? &LIWP->getLoopInfo() : nullptr; + bool PreserveLCSSA = P->mustPreserveAnalysisID(LCSSAID); + auto Options = CriticalEdgeSplittingOptions(AA, DT, LI); + if (PreserveLCSSA) + Options.setPreserveLCSSA(); + + // If this is a critical edge, let SplitCriticalEdge do it. + TerminatorInst *LatchTerm = BB->getTerminator(); + if (SplitCriticalEdge(LatchTerm, SuccNum, Options)) + return LatchTerm->getSuccessor(SuccNum); // If the edge isn't critical, then BB has a single successor or Succ has a // single pred. Split the block. @@ -261,13 +266,15 @@ BasicBlock *llvm::SplitEdge(BasicBlock *BB, BasicBlock *Succ, Pass *P) { return SplitBlock(BB, BB->getTerminator(), DT, LI); } -unsigned llvm::SplitAllCriticalEdges(Function &F, Pass *P) { +unsigned +llvm::SplitAllCriticalEdges(Function &F, + const CriticalEdgeSplittingOptions &Options) { unsigned NumBroken = 0; for (Function::iterator I = F.begin(), E = F.end(); I != E; ++I) { TerminatorInst *TI = I->getTerminator(); if (TI->getNumSuccessors() > 1 && !isa(TI)) for (unsigned i = 0, e = TI->getNumSuccessors(); i != e; ++i) - if (SplitCriticalEdge(TI, i, P)) + if (SplitCriticalEdge(TI, i, Options)) ++NumBroken; } return NumBroken; diff --git a/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp b/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp index 932411933cc0..7e83c9eeceb7 100644 --- a/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp +++ b/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp @@ -42,7 +42,12 @@ namespace { } bool runOnFunction(Function &F) override { - unsigned N = SplitAllCriticalEdges(F, this); + auto *DTWP = getAnalysisIfAvailable(); + auto *DT = DTWP ? &DTWP->getDomTree() : nullptr; + auto *LIWP = getAnalysisIfAvailable(); + auto *LI = LIWP ? &LIWP->getLoopInfo() : nullptr; + unsigned N = + SplitAllCriticalEdges(F, CriticalEdgeSplittingOptions(DT, LI)); NumBroken += N; return N > 0; } @@ -126,10 +131,9 @@ static void createPHIsForSplitLoopExit(ArrayRef Preds, /// to. /// BasicBlock *llvm::SplitCriticalEdge(TerminatorInst *TI, unsigned SuccNum, - Pass *P, bool MergeIdenticalEdges, - bool DontDeleteUselessPhis, - bool SplitLandingPads) { - if (!isCriticalEdge(TI, SuccNum, MergeIdenticalEdges)) return nullptr; + const CriticalEdgeSplittingOptions &Options) { + if (!isCriticalEdge(TI, SuccNum, Options.MergeIdenticalEdges)) + return nullptr; assert(!isa(TI) && "Cannot split critical edge from IndirectBrInst"); @@ -180,33 +184,22 @@ BasicBlock *llvm::SplitCriticalEdge(TerminatorInst *TI, unsigned SuccNum, // If there are any other edges from TIBB to DestBB, update those to go // through the split block, making those edges non-critical as well (and // reducing the number of phi entries in the DestBB if relevant). - if (MergeIdenticalEdges) { + if (Options.MergeIdenticalEdges) { for (unsigned i = SuccNum+1, e = TI->getNumSuccessors(); i != e; ++i) { if (TI->getSuccessor(i) != DestBB) continue; // Remove an entry for TIBB from DestBB phi nodes. - DestBB->removePredecessor(TIBB, DontDeleteUselessPhis); + DestBB->removePredecessor(TIBB, Options.DontDeleteUselessPHIs); // We found another edge to DestBB, go to NewBB instead. TI->setSuccessor(i, NewBB); } } - - - // If we don't have a pass object, we can't update anything... - if (!P) return NewBB; - - - auto *AA = P->getAnalysisIfAvailable(); - DominatorTreeWrapperPass *DTWP = - P->getAnalysisIfAvailable(); - DominatorTree *DT = DTWP ? &DTWP->getDomTree() : nullptr; - auto *LIWP = P->getAnalysisIfAvailable(); - LoopInfo *LI = LIWP ? &LIWP->getLoopInfo() : nullptr; - bool PreserveLCSSA = P->mustPreserveAnalysisID(LCSSAID); - // If we have nothing to update, just return. + auto *AA = Options.AA; + auto *DT = Options.DT; + auto *LI = Options.LI; if (!DT && !LI) return NewBB; @@ -299,7 +292,7 @@ BasicBlock *llvm::SplitCriticalEdge(TerminatorInst *TI, unsigned SuccNum, "Split point for loop exit is contained in loop!"); // Update LCSSA form in the newly created exit block. - if (PreserveLCSSA) { + if (Options.PreserveLCSSA) { createPHIsForSplitLoopExit(TIBB, NewBB, DestBB); } @@ -329,8 +322,8 @@ BasicBlock *llvm::SplitCriticalEdge(TerminatorInst *TI, unsigned SuccNum, assert(!DestBB->isLandingPad() && "We don't split edges to landing pads!"); BasicBlock *NewExitBB = SplitBlockPredecessors( - DestBB, LoopPreds, "split", AA, DT, LI, PreserveLCSSA); - if (PreserveLCSSA) + DestBB, LoopPreds, "split", AA, DT, LI, Options.PreserveLCSSA); + if (Options.PreserveLCSSA) createPHIsForSplitLoopExit(LoopPreds, NewExitBB, DestBB); } }