[llvm] r213474 - [C++11] Add predecessors(BasicBlock *) / successors(BasicBlock *) iterator ranges.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Jul 21 10:31:10 PDT 2014
> On 2014-Jul-20, at 02:10, Manuel Jacob <me at manueljacob.de> wrote:
>
> Author: mjacob
> Date: Sun Jul 20 04:10:11 2014
> New Revision: 213474
>
> URL: http://llvm.org/viewvc/llvm-project?rev=213474&view=rev
> Log:
> [C++11] Add predecessors(BasicBlock *) / successors(BasicBlock *) iterator ranges.
>
> Summary: This patch introduces two new iterator ranges and updates existing code to use it. No functional change intended.
>
> Test Plan: All tests (make check-all) still pass.
>
I've reverted this (and its follow-up) in r213562, since it caused our
internal LTO bootstrap builder to miscompile. Unfortunately, our public
LTO bootstrap builder is a day or two behind so I can't point you at a
failure, but I've reviewed this and highlighted one bug below. There
may be more.
This is really hard to review carefully -- I'd appreciate it if when you
recommit you do it in stages, so that if there are more bugs they're on
blamelists for different builds.
BTW, I don't see the value in these mass refactorings to use C++11
range-based for loops. While adding the API is obviously good, changing
everything to use it means that these mistakes are going to creep in,
and it's hard enough to review these that they won't get many eyes.
I think it would be better if the next person to touch the code cleaned
it up (where appropriate). If they're touching the code anyway, they'll
have the time to understand the logic and be careful about it.
> Reviewers: dblaikie
>
> Reviewed By: dblaikie
>
> Subscribers: llvm-commits
>
> Differential Revision: http://reviews.llvm.org/D4481
>
> Modified:
> llvm/trunk/docs/ProgrammersManual.rst
> llvm/trunk/include/llvm/IR/CFG.h
> llvm/trunk/lib/Analysis/BranchProbabilityInfo.cpp
> llvm/trunk/lib/Analysis/Interval.cpp
> llvm/trunk/lib/Analysis/LazyValueInfo.cpp
> llvm/trunk/lib/Analysis/LoopInfo.cpp
> llvm/trunk/lib/Analysis/ScalarEvolution.cpp
> llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp
> llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> llvm/trunk/lib/CodeGen/SjLjEHPrepare.cpp
> llvm/trunk/lib/CodeGen/UnreachableBlockElim.cpp
> llvm/trunk/lib/IR/BasicBlock.cpp
> llvm/trunk/lib/IR/Dominators.cpp
> llvm/trunk/lib/IR/Verifier.cpp
> llvm/trunk/lib/Transforms/IPO/ArgumentPromotion.cpp
> llvm/trunk/lib/Transforms/IPO/LoopExtractor.cpp
> llvm/trunk/lib/Transforms/IPO/PartialInlining.cpp
> llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
> llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp
> llvm/trunk/lib/Transforms/Scalar/GVN.cpp
> llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp
> llvm/trunk/lib/Transforms/Scalar/LoopInstSimplify.cpp
> llvm/trunk/lib/Transforms/Scalar/LoopUnswitch.cpp
> llvm/trunk/lib/Transforms/Scalar/SampleProfile.cpp
> llvm/trunk/lib/Transforms/Scalar/Sink.cpp
> llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp
> llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp
> llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp
> llvm/trunk/lib/Transforms/Utils/BreakCriticalEdges.cpp
> llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp
> llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp
> llvm/trunk/lib/Transforms/Utils/Local.cpp
> llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp
> llvm/trunk/lib/Transforms/Utils/LoopUnroll.cpp
> llvm/trunk/lib/Transforms/Utils/LoopUnrollRuntime.cpp
> llvm/trunk/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
> llvm/trunk/lib/Transforms/Utils/SSAUpdater.cpp
> llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
> llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp
> llvm/trunk/tools/bugpoint/CrashDebugger.cpp
> llvm/trunk/tools/llvm-diff/DifferenceEngine.cpp
>
>
...
> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=213474&r1=213473&r2=213474&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Sun Jul 20 04:10:11 2014
> @@ -130,9 +130,9 @@ static bool SafeToMergeTerminators(Termi
> BasicBlock *SI2BB = SI2->getParent();
> SmallPtrSet<BasicBlock*, 16> SI1Succs(succ_begin(SI1BB), succ_end(SI1BB));
>
> - for (succ_iterator I = succ_begin(SI2BB), E = succ_end(SI2BB); I != E; ++I)
> - if (SI1Succs.count(*I))
> - for (BasicBlock::iterator BBI = (*I)->begin();
> + for (BasicBlock *Succ : successors(SI2BB))
> + if (SI1Succs.count(Succ))
> + for (BasicBlock::iterator BBI = Succ->begin();
> isa<PHINode>(BBI); ++BBI) {
> PHINode *PN = cast<PHINode>(BBI);
> if (PN->getIncomingValueForBlock(SI1BB) !=
> @@ -171,9 +171,9 @@ static bool isProfitableToFoldUnconditio
> BasicBlock *SI1BB = SI1->getParent();
> BasicBlock *SI2BB = SI2->getParent();
> SmallPtrSet<BasicBlock*, 16> SI1Succs(succ_begin(SI1BB), succ_end(SI1BB));
> - for (succ_iterator I = succ_begin(SI2BB), E = succ_end(SI2BB); I != E; ++I)
> - if (SI1Succs.count(*I))
> - for (BasicBlock::iterator BBI = (*I)->begin();
> + for (BasicBlock *Succ : successors(SI2BB))
> + if (SI1Succs.count(Succ))
> + for (BasicBlock::iterator BBI = Succ->begin();
> isa<PHINode>(BBI); ++BBI) {
> PHINode *PN = cast<PHINode>(BBI);
> if (PN->getIncomingValueForBlock(SI1BB) != Cond ||
> @@ -683,9 +683,9 @@ SimplifyEqualityComparisonWithOnlyPredec
>
> // Remove PHI node entries for dead edges.
> BasicBlock *CheckEdge = TheRealDest;
> - for (succ_iterator SI = succ_begin(TIBB), e = succ_end(TIBB); SI != e; ++SI)
> - if (*SI != CheckEdge)
> - (*SI)->removePredecessor(TIBB);
> + for (BasicBlock *Succ : successors(TIBB))
> + if (Succ != CheckEdge)
> + Succ->removePredecessor(TIBB);
> else
> CheckEdge = nullptr;
>
> @@ -981,9 +981,9 @@ bool SimplifyCFGOpt::FoldValueComparison
> // to put the select in this case.
> static bool isSafeToHoistInvoke(BasicBlock *BB1, BasicBlock *BB2,
> Instruction *I1, Instruction *I2) {
> - for (succ_iterator SI = succ_begin(BB1), E = succ_end(BB1); SI != E; ++SI) {
> + for (BasicBlock *Succ : successors(BB1)) {
> PHINode *PN;
> - for (BasicBlock::iterator BBI = SI->begin();
> + for (BasicBlock::iterator BBI = Succ->begin();
> (PN = dyn_cast<PHINode>(BBI)); ++BBI) {
> Value *BB1V = PN->getIncomingValueForBlock(BB1);
> Value *BB2V = PN->getIncomingValueForBlock(BB2);
> @@ -1063,9 +1063,9 @@ HoistTerminator:
> if (isa<InvokeInst>(I1) && !isSafeToHoistInvoke(BB1, BB2, I1, I2))
> return Changed;
>
> - for (succ_iterator SI = succ_begin(BB1), E = succ_end(BB1); SI != E; ++SI) {
> + for (BasicBlock *Succ : successors(BB1)) {
> PHINode *PN;
> - for (BasicBlock::iterator BBI = SI->begin();
> + for (BasicBlock::iterator BBI = Succ->begin();
> (PN = dyn_cast<PHINode>(BBI)); ++BBI) {
> Value *BB1V = PN->getIncomingValueForBlock(BB1);
> Value *BB2V = PN->getIncomingValueForBlock(BB2);
> @@ -1094,9 +1094,9 @@ HoistTerminator:
> // them. If they do, all PHI entries for BB1/BB2 must agree for all PHI
> // nodes, so we insert select instruction to compute the final result.
> std::map<std::pair<Value*,Value*>, SelectInst*> InsertedSelects;
> - for (succ_iterator SI = succ_begin(BB1), E = succ_end(BB1); SI != E; ++SI) {
> + for (BasicBlock *Succ : successors(BB1)) {
> PHINode *PN;
> - for (BasicBlock::iterator BBI = SI->begin();
> + for (BasicBlock::iterator BBI = Succ->begin();
> (PN = dyn_cast<PHINode>(BBI)); ++BBI) {
> Value *BB1V = PN->getIncomingValueForBlock(BB1);
> Value *BB2V = PN->getIncomingValueForBlock(BB2);
> @@ -1118,8 +1118,8 @@ HoistTerminator:
> }
>
> // Update any PHI nodes in our new successors.
> - for (succ_iterator SI = succ_begin(BB1), E = succ_end(BB1); SI != E; ++SI)
> - AddPredecessorToBlock(*SI, BIParent, BB1);
> + for (BasicBlock *Succ : successors(BB1))
> + AddPredecessorToBlock(Succ, BIParent, BB1);
>
> EraseTerminatorInstAndDCECond(BI);
> return true;
> @@ -2051,8 +2051,7 @@ bool llvm::FoldBranchToCommonDest(Branch
> if (TrueDest == BB || FalseDest == BB)
> return false;
>
> - for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI) {
> - BasicBlock *PredBlock = *PI;
> + for (BasicBlock *PredBlock : predecessors(BB)) {
> BranchInst *PBI = dyn_cast<BranchInst>(PredBlock->getTerminator());
>
> // Check that we have two conditional branches. If there is a PHI node in
> @@ -2885,8 +2884,8 @@ bool SimplifyCFGOpt::SimplifyResume(Resu
> // Turn all invokes that unwind here into calls and delete the basic block.
> bool InvokeRequiresTableEntry = false;
> bool Changed = false;
> - for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE;) {
> - InvokeInst *II = cast<InvokeInst>((*PI++)->getTerminator());
> + for (BasicBlock *Pred : predecessors(BB)) {
> + InvokeInst *II = cast<InvokeInst>(Pred->getTerminator());
This is a bug. `II` gets deleted below. `PI` needs to be incremented
*now*, not at the end of the block.
>
> if (II->hasFnAttr(Attribute::UWTable)) {
> // Don't remove an `invoke' instruction if the ABI requires an entry into
> @@ -2933,8 +2932,7 @@ bool SimplifyCFGOpt::SimplifyReturn(Retu
> // Find predecessors that end with branches.
> SmallVector<BasicBlock*, 8> UncondBranchPreds;
> SmallVector<BranchInst*, 8> CondBranchPreds;
> - for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI) {
> - BasicBlock *P = *PI;
> + for (BasicBlock *P : predecessors(BB)) {
> TerminatorInst *PTI = P->getTerminator();
> if (BranchInst *BI = dyn_cast<BranchInst>(PTI)) {
> if (BI->isUnconditional())
> @@ -4100,8 +4098,8 @@ bool SimplifyCFGOpt::SimplifyCondBranch(
> return SimplifyCFG(BB, TTI, DL) | true;
>
> // Scan predecessor blocks for conditional branches.
> - for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI)
> - if (BranchInst *PBI = dyn_cast<BranchInst>((*PI)->getTerminator()))
> + for (BasicBlock *Pred : predecessors(BB))
> + if (BranchInst *PBI = dyn_cast<BranchInst>(Pred->getTerminator()))
> if (PBI != BI && PBI->isConditional())
> if (SimplifyCondBranchToCondBranch(PBI, BI))
> return SimplifyCFG(BB, TTI, DL) | true;
>
> Modified: llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp?rev=213474&r1=213473&r2=213474&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp (original)
> +++ llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp Sun Jul 20 04:10:11 2014
> @@ -2934,9 +2934,8 @@ InnerLoopVectorizer::createBlockInMask(B
> Value *Zero = ConstantInt::get(IntegerType::getInt1Ty(BB->getContext()), 0);
> VectorParts BlockMask = getVectorValue(Zero);
>
> - // For each pred:
> - for (pred_iterator it = pred_begin(BB), e = pred_end(BB); it != e; ++it) {
> - VectorParts EM = createEdgeMask(*it, BB);
> + for (BasicBlock *Pred : predecessors(BB)) {
> + VectorParts EM = createEdgeMask(Pred, BB);
> for (unsigned part = 0; part < UF; ++part)
> BlockMask[part] = Builder.CreateOr(BlockMask[part], EM[part]);
> }
>
> Modified: llvm/trunk/tools/bugpoint/CrashDebugger.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/bugpoint/CrashDebugger.cpp?rev=213474&r1=213473&r2=213474&view=diff
> ==============================================================================
> --- llvm/trunk/tools/bugpoint/CrashDebugger.cpp (original)
> +++ llvm/trunk/tools/bugpoint/CrashDebugger.cpp Sun Jul 20 04:10:11 2014
> @@ -292,8 +292,8 @@ bool ReduceCrashingBlocks::TestBlocks(st
> if (!Blocks.count(BB) && BB->getTerminator()->getNumSuccessors()) {
> // Loop over all of the successors of this block, deleting any PHI nodes
> // that might include it.
> - for (succ_iterator SI = succ_begin(BB), E = succ_end(BB); SI != E; ++SI)
> - (*SI)->removePredecessor(BB);
> + for (BasicBlock *Succ : successors(BB))
> + Succ->removePredecessor(BB);
>
> TerminatorInst *BBTerm = BB->getTerminator();
>
>
> Modified: llvm/trunk/tools/llvm-diff/DifferenceEngine.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-diff/DifferenceEngine.cpp?rev=213474&r1=213473&r2=213474&view=diff
> ==============================================================================
> --- llvm/trunk/tools/llvm-diff/DifferenceEngine.cpp (original)
> +++ llvm/trunk/tools/llvm-diff/DifferenceEngine.cpp Sun Jul 20 04:10:11 2014
> @@ -125,8 +125,8 @@ class FunctionDifferenceEngine {
>
> unsigned getUnprocPredCount(BasicBlock *Block) const {
> unsigned Count = 0;
> - for (pred_iterator I = pred_begin(Block), E = pred_end(Block); I != E; ++I)
> - if (!Blocks.count(*I)) Count++;
> + for (BasicBlock *Pred : predecessors(Block))
> + if (!Blocks.count(Pred)) Count++;
> return Count;
> }
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list