From a2c9af02097b085baab54291a18d23f2201e050e Mon Sep 17 00:00:00 2001 From: Michael Zolotukhin Date: Fri, 20 Apr 2018 13:34:32 +0000 Subject: [PATCH] Revert "Revert r330403 and r330413." Reapply the patches with a fix. Thanks Ilya and Hans for the reproducer! This reverts commit r330416. The issue was that removing predecessors invalidated uses that we stored for rewrite. The fix is to finish manipulating with CFG before we select uses for rewrite. llvm-svn: 330431 --- .../llvm/Transforms/Utils/SSAUpdaterBulk.h | 7 ++- llvm/lib/Transforms/Scalar/JumpThreading.cpp | 56 ++++++++++--------- llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp | 25 ++++----- .../Transforms/JumpThreading/removed-use.ll | 54 ++++++++++++++++++ .../Transforms/Utils/SSAUpdaterBulk.cpp | 38 ++++++------- 5 files changed, 119 insertions(+), 61 deletions(-) create mode 100644 llvm/test/Transforms/JumpThreading/removed-use.ll diff --git a/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h b/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h index aed370b4b593..53a608f01804 100644 --- a/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h +++ b/llvm/include/llvm/Transforms/Utils/SSAUpdaterBulk.h @@ -47,7 +47,7 @@ class SSAUpdaterBulk { RewriteInfo(){}; RewriteInfo(StringRef &N, Type *T) : Name(N), Ty(T){}; }; - DenseMap Rewrites; + SmallVector Rewrites; PredIteratorCache PredCache; @@ -60,8 +60,9 @@ public: ~SSAUpdaterBulk(){}; /// Add a new variable to the SSA rewriter. This needs to be called before - /// AddAvailableValue or AddUse calls. - void AddVariable(unsigned Var, StringRef Name, Type *Ty); + /// AddAvailableValue or AddUse calls. The return value is the variable ID, + /// which needs to be passed to AddAvailableValue and AddUse. + unsigned AddVariable(StringRef Name, Type *Ty); /// Indicate that a rewritten value is available in the specified block with /// the specified value. diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp index 29c707799bd5..796b6abcbfd7 100644 --- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp +++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp @@ -66,6 +66,7 @@ #include "llvm/Transforms/Utils/BasicBlockUtils.h" #include "llvm/Transforms/Utils/Cloning.h" #include "llvm/Transforms/Utils/SSAUpdater.h" +#include "llvm/Transforms/Utils/SSAUpdaterBulk.h" #include "llvm/Transforms/Utils/ValueMapper.h" #include #include @@ -1985,19 +1986,33 @@ bool JumpThreadingPass::ThreadEdge(BasicBlock *BB, // PHI nodes for NewBB now. AddPHINodeEntriesForMappedBlock(SuccBB, BB, NewBB, ValueMapping); + // Update the terminator of PredBB to jump to NewBB instead of BB. This + // eliminates predecessors from BB, which requires us to simplify any PHI + // nodes in BB. + TerminatorInst *PredTerm = PredBB->getTerminator(); + for (unsigned i = 0, e = PredTerm->getNumSuccessors(); i != e; ++i) + if (PredTerm->getSuccessor(i) == BB) { + BB->removePredecessor(PredBB, true); + PredTerm->setSuccessor(i, NewBB); + } + // If there were values defined in BB that are used outside the block, then we // now have to update all uses of the value to use either the original value, // the cloned value, or some PHI derived value. This can require arbitrary // PHI insertion, of which we are prepared to do, clean these up now. - SSAUpdater SSAUpdate; - SmallVector UsesToRename; + SSAUpdaterBulk SSAUpdate; + for (Instruction &I : *BB) { + SmallVector UsesToRename; + // Scan all uses of this instruction to see if it is used outside of its - // block, and if so, record them in UsesToRename. + // block, and if so, record them in UsesToRename. Also, skip phi operands + // from PredBB - we'll remove them anyway. for (Use &U : I.uses()) { Instruction *User = cast(U.getUser()); if (PHINode *UserPN = dyn_cast(User)) { - if (UserPN->getIncomingBlock(U) == BB) + if (UserPN->getIncomingBlock(U) == BB || + UserPN->getIncomingBlock(U) == PredBB) continue; } else if (User->getParent() == BB) continue; @@ -2008,35 +2023,24 @@ bool JumpThreadingPass::ThreadEdge(BasicBlock *BB, // If there are no uses outside the block, we're done with this instruction. if (UsesToRename.empty()) continue; + unsigned VarNum = SSAUpdate.AddVariable(I.getName(), I.getType()); - DEBUG(dbgs() << "JT: Renaming non-local uses of: " << I << "\n"); - - // We found a use of I outside of BB. Rename all uses of I that are outside - // its block to be uses of the appropriate PHI node etc. See ValuesInBlocks - // with the two values we know. - SSAUpdate.Initialize(I.getType(), I.getName()); - SSAUpdate.AddAvailableValue(BB, &I); - SSAUpdate.AddAvailableValue(NewBB, ValueMapping[&I]); - - while (!UsesToRename.empty()) - SSAUpdate.RewriteUse(*UsesToRename.pop_back_val()); - DEBUG(dbgs() << "\n"); + // We found a use of I outside of BB - we need to rename all uses of I that + // are outside its block to be uses of the appropriate PHI node etc. + SSAUpdate.AddAvailableValue(VarNum, BB, &I); + SSAUpdate.AddAvailableValue(VarNum, NewBB, ValueMapping[&I]); + for (auto *U : UsesToRename) + SSAUpdate.AddUse(VarNum, U); } - // Ok, NewBB is good to go. Update the terminator of PredBB to jump to - // NewBB instead of BB. This eliminates predecessors from BB, which requires - // us to simplify any PHI nodes in BB. - TerminatorInst *PredTerm = PredBB->getTerminator(); - for (unsigned i = 0, e = PredTerm->getNumSuccessors(); i != e; ++i) - if (PredTerm->getSuccessor(i) == BB) { - BB->removePredecessor(PredBB, true); - PredTerm->setSuccessor(i, NewBB); - } - DDT->applyUpdates({{DominatorTree::Insert, NewBB, SuccBB}, {DominatorTree::Insert, PredBB, NewBB}, {DominatorTree::Delete, PredBB, BB}}); + // Apply all updates we queued with DDT and get the updated Dominator Tree. + DominatorTree *DT = &DDT->flush(); + SSAUpdate.RewriteAllUses(DT); + // At this point, the IR is fully up to date and consistent. Do a quick scan // over the new instructions and zap any that are constants or dead. This // frequently happens because of phi translation. diff --git a/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp b/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp index dbc671cc16ff..61ceb9c92778 100644 --- a/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp +++ b/llvm/lib/Transforms/Utils/SSAUpdaterBulk.cpp @@ -38,18 +38,19 @@ static BasicBlock *getUserBB(Use *U) { /// Add a new variable to the SSA rewriter. This needs to be called before /// AddAvailableValue or AddUse calls. -void SSAUpdaterBulk::AddVariable(unsigned Var, StringRef Name, Type *Ty) { - assert(Rewrites.find(Var) == Rewrites.end() && "Variable added twice!"); +unsigned SSAUpdaterBulk::AddVariable(StringRef Name, Type *Ty) { + unsigned Var = Rewrites.size(); DEBUG(dbgs() << "SSAUpdater: Var=" << Var << ": initialized with Ty = " << *Ty << ", Name = " << Name << "\n"); RewriteInfo RI(Name, Ty); - Rewrites[Var] = RI; + Rewrites.push_back(RI); + return Var; } /// Indicate that a rewritten value is available in the specified block with the /// specified value. void SSAUpdaterBulk::AddAvailableValue(unsigned Var, BasicBlock *BB, Value *V) { - assert(Rewrites.find(Var) != Rewrites.end() && "Should add variable first!"); + assert(Var < Rewrites.size() && "Variable not found!"); DEBUG(dbgs() << "SSAUpdater: Var=" << Var << ": added new available value" << *V << " in " << BB->getName() << "\n"); Rewrites[Var].Defines[BB] = V; @@ -58,7 +59,7 @@ void SSAUpdaterBulk::AddAvailableValue(unsigned Var, BasicBlock *BB, Value *V) { /// Record a use of the symbolic value. This use will be updated with a /// rewritten value when RewriteAllUses is called. void SSAUpdaterBulk::AddUse(unsigned Var, Use *U) { - assert(Rewrites.find(Var) != Rewrites.end() && "Should add variable first!"); + assert(Var < Rewrites.size() && "Variable not found!"); DEBUG(dbgs() << "SSAUpdater: Var=" << Var << ": added a use" << *U->get() << " in " << getUserBB(U)->getName() << "\n"); Rewrites[Var].Uses.push_back(U); @@ -67,7 +68,7 @@ void SSAUpdaterBulk::AddUse(unsigned Var, Use *U) { /// Return true if the SSAUpdater already has a value for the specified variable /// in the specified block. bool SSAUpdaterBulk::HasValueForBlock(unsigned Var, BasicBlock *BB) { - return Rewrites.count(Var) ? Rewrites[Var].Defines.count(BB) : false; + return (Var < Rewrites.size()) ? Rewrites[Var].Defines.count(BB) : false; } // Compute value at the given block BB. We either should already know it, or we @@ -126,16 +127,14 @@ ComputeLiveInBlocks(const SmallPtrSetImpl &UsingBlocks, /// requested uses update. void SSAUpdaterBulk::RewriteAllUses(DominatorTree *DT, SmallVectorImpl *InsertedPHIs) { - for (auto &P : Rewrites) { + for (auto &R : Rewrites) { // Compute locations for new phi-nodes. // For that we need to initialize DefBlocks from definitions in R.Defines, // UsingBlocks from uses in R.Uses, then compute LiveInBlocks, and then use // this set for computing iterated dominance frontier (IDF). // The IDF blocks are the blocks where we need to insert new phi-nodes. ForwardIDFCalculator IDF(*DT); - RewriteInfo &R = P.second; - DEBUG(dbgs() << "SSAUpdater: Var=" << P.first << ": rewriting " - << R.Uses.size() << " use(s)\n"); + DEBUG(dbgs() << "SSAUpdater: rewriting " << R.Uses.size() << " use(s)\n"); SmallPtrSet DefBlocks; for (auto &Def : R.Defines) @@ -165,7 +164,7 @@ void SSAUpdaterBulk::RewriteAllUses(DominatorTree *DT, } // Fill in arguments of the inserted PHIs. - for (auto PN : InsertedPHIsForVar) { + for (auto *PN : InsertedPHIsForVar) { BasicBlock *PBB = PN->getParent(); for (BasicBlock *Pred : PredCache.get(PBB)) PN->addIncoming(computeValueAt(Pred, R, DT), Pred); @@ -182,8 +181,8 @@ void SSAUpdaterBulk::RewriteAllUses(DominatorTree *DT, // Notify that users of the existing value that it is being replaced. if (OldVal != V && OldVal->hasValueHandle()) ValueHandleBase::ValueIsRAUWd(OldVal, V); - DEBUG(dbgs() << "SSAUpdater: Var=" << P.first << ": replacing" << *OldVal - << " with " << *V << "\n"); + DEBUG(dbgs() << "SSAUpdater: replacing " << *OldVal << " with " << *V + << "\n"); U->set(V); } } diff --git a/llvm/test/Transforms/JumpThreading/removed-use.ll b/llvm/test/Transforms/JumpThreading/removed-use.ll new file mode 100644 index 000000000000..4866a11b9c61 --- /dev/null +++ b/llvm/test/Transforms/JumpThreading/removed-use.ll @@ -0,0 +1,54 @@ +; RUN: opt -S < %s -jump-threading | FileCheck %s +; CHECK-LABEL: @foo +; CHECK: bb6: +; CHECK-NEXT: ret void +; CHECK: bb3: +; CHECK: br label %bb3 +define void @foo() { +entry: + br i1 true, label %bb6, label %bb3 + +bb3: + %x0 = phi i32 [ undef, %entry ], [ %x1, %bb5 ] + %y = and i64 undef, 1 + %p = icmp ne i64 %y, 0 + br i1 %p, label %bb4, label %bb5 + +bb4: + br label %bb5 + +bb5: + %x1 = phi i32 [ %x0, %bb3 ], [ %x0, %bb4 ] + %z = phi i32 [ 0, %bb3 ], [ 1, %bb4 ] + %q = icmp eq i32 %z, 0 + br i1 %q, label %bb3, label %bb6 + +bb6: + ret void +} + +; CHECK-LABEL: bar@ +; Just check that we don't crash on this test. +define void @bar(i1 %p) { +entry: + br i1 false, label %bb2, label %exit + +bb2: + %x0 = phi i32 [ undef, %entry ], [ %x1, %bb5 ] + br i1 %p, label %bb3, label %bb4 + +bb3: + br label %bb5 + +bb4: + br label %bb5 + +bb5: + %x1 = phi i32 [ %x0, %bb3 ], [ 0, %bb4 ] + switch i32 %x1, label %exit [ + i32 10, label %bb2 + ] + +exit: + ret void +} diff --git a/llvm/unittests/Transforms/Utils/SSAUpdaterBulk.cpp b/llvm/unittests/Transforms/Utils/SSAUpdaterBulk.cpp index 6a8adf9ef37c..61cbcb7b1a77 100644 --- a/llvm/unittests/Transforms/Utils/SSAUpdaterBulk.cpp +++ b/llvm/unittests/Transforms/Utils/SSAUpdaterBulk.cpp @@ -73,17 +73,17 @@ TEST(SSAUpdaterBulk, SimpleMerge) { // SSAUpdater should insert into %merge. // Intentionally don't touch %8 to see that SSAUpdater only changes // instructions that were explicitly specified. - Updater.AddVariable(0, "a", I32Ty); - Updater.AddAvailableValue(0, TrueBB, AddOp1); - Updater.AddAvailableValue(0, FalseBB, AddOp2); - Updater.AddUse(0, &I1->getOperandUse(0)); - Updater.AddUse(0, &I2->getOperandUse(0)); + unsigned VarNum = Updater.AddVariable("a", I32Ty); + Updater.AddAvailableValue(VarNum, TrueBB, AddOp1); + Updater.AddAvailableValue(VarNum, FalseBB, AddOp2); + Updater.AddUse(VarNum, &I1->getOperandUse(0)); + Updater.AddUse(VarNum, &I2->getOperandUse(0)); - Updater.AddVariable(1, "b", I32Ty); - Updater.AddAvailableValue(1, TrueBB, SubOp1); - Updater.AddAvailableValue(1, FalseBB, SubOp2); - Updater.AddUse(1, &I3->getOperandUse(0)); - Updater.AddUse(1, &I3->getOperandUse(1)); + VarNum = Updater.AddVariable("b", I32Ty); + Updater.AddAvailableValue(VarNum, TrueBB, SubOp1); + Updater.AddAvailableValue(VarNum, FalseBB, SubOp2); + Updater.AddUse(VarNum, &I3->getOperandUse(0)); + Updater.AddUse(VarNum, &I3->getOperandUse(1)); DominatorTree DT(*F); Updater.RewriteAllUses(&DT); @@ -161,19 +161,19 @@ TEST(SSAUpdaterBulk, Irreducible) { // No other rewrites should be made. // Add use in %3. - Updater.AddVariable(0, "c", I32Ty); - Updater.AddAvailableValue(0, IfBB, AddOp1); - Updater.AddUse(0, &I1->getOperandUse(0)); + unsigned VarNum = Updater.AddVariable("c", I32Ty); + Updater.AddAvailableValue(VarNum, IfBB, AddOp1); + Updater.AddUse(VarNum, &I1->getOperandUse(0)); // Add use in %4. - Updater.AddVariable(1, "b", I32Ty); - Updater.AddAvailableValue(1, LoopStartBB, AddOp2); - Updater.AddUse(1, &I2->getOperandUse(0)); + VarNum = Updater.AddVariable("b", I32Ty); + Updater.AddAvailableValue(VarNum, LoopStartBB, AddOp2); + Updater.AddUse(VarNum, &I2->getOperandUse(0)); // Add use in the return instruction. - Updater.AddVariable(2, "a", I32Ty); - Updater.AddAvailableValue(2, &F->getEntryBlock(), FirstArg); - Updater.AddUse(2, &Return->getOperandUse(0)); + VarNum = Updater.AddVariable("a", I32Ty); + Updater.AddAvailableValue(VarNum, &F->getEntryBlock(), FirstArg); + Updater.AddUse(VarNum, &Return->getOperandUse(0)); // Save all inserted phis into a vector. SmallVector Inserted;