From 8e71a936a4c03853038ba70233ec4f8b50bed6b2 Mon Sep 17 00:00:00 2001 From: Ilya Biryukov Date: Fri, 20 Apr 2018 10:52:54 +0000 Subject: [PATCH] Revert r330403 and r330413. Revert r330413: "[SSAUpdaterBulk] Use SmallVector instead of DenseMap for storing rewrites." Revert r330403 "Reapply "[PR16756] Use SSAUpdaterBulk in JumpThreading." one more time." r330403 commit seems to crash clang during our integrate while doing PGO build with the following stacktrace: #2 llvm::SSAUpdaterBulk::RewriteAllUses(llvm::DominatorTree*, llvm::SmallVectorImpl*) #3 llvm::JumpThreadingPass::ThreadEdge(llvm::BasicBlock*, llvm::SmallVectorImpl const&, llvm::BasicBlock*) #4 llvm::JumpThreadingPass::ProcessThreadableEdges(llvm::Value*, llvm::BasicBlock*, llvm::jumpthreading::ConstantPreference, llvm::Instruction*) #5 llvm::JumpThreadingPass::ProcessBlock(llvm::BasicBlock*) The crash happens while compiling 'lib/Analysis/CallGraph.cpp'. r3340413 is reverted due to conflicting changes. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@330416 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../llvm/Transforms/Utils/SSAUpdaterBulk.h | 7 ++-- lib/Transforms/Scalar/JumpThreading.cpp | 36 ++++++++--------- lib/Transforms/Utils/SSAUpdaterBulk.cpp | 25 ++++++------ test/Transforms/JumpThreading/removed-use.ll | 28 ------------- unittests/Transforms/Utils/SSAUpdaterBulk.cpp | 40 +++++++++---------- 5 files changed, 52 insertions(+), 84 deletions(-) delete mode 100644 test/Transforms/JumpThreading/removed-use.ll diff --git a/include/llvm/Transforms/Utils/SSAUpdaterBulk.h b/include/llvm/Transforms/Utils/SSAUpdaterBulk.h index 53a608f01804..aed370b4b593 100644 --- a/include/llvm/Transforms/Utils/SSAUpdaterBulk.h +++ b/include/llvm/Transforms/Utils/SSAUpdaterBulk.h @@ -47,7 +47,7 @@ class SSAUpdaterBulk { RewriteInfo(){}; RewriteInfo(StringRef &N, Type *T) : Name(N), Ty(T){}; }; - SmallVector Rewrites; + DenseMap Rewrites; PredIteratorCache PredCache; @@ -60,9 +60,8 @@ class SSAUpdaterBulk { ~SSAUpdaterBulk(){}; /// Add a new variable to the SSA rewriter. This needs to be called before - /// 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); + /// AddAvailableValue or AddUse calls. + void AddVariable(unsigned Var, StringRef Name, Type *Ty); /// Indicate that a rewritten value is available in the specified block with /// the specified value. diff --git a/lib/Transforms/Scalar/JumpThreading.cpp b/lib/Transforms/Scalar/JumpThreading.cpp index b54bbbc928a9..29c707799bd5 100644 --- a/lib/Transforms/Scalar/JumpThreading.cpp +++ b/lib/Transforms/Scalar/JumpThreading.cpp @@ -66,7 +66,6 @@ #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 @@ -1990,19 +1989,15 @@ bool JumpThreadingPass::ThreadEdge(BasicBlock *BB, // 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. - SSAUpdaterBulk SSAUpdate; - + SSAUpdater SSAUpdate; + SmallVector UsesToRename; 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. Also, skip phi operands - // from PredBB - we'll remove them anyway. + // block, and if so, record them in UsesToRename. for (Use &U : I.uses()) { Instruction *User = cast(U.getUser()); if (PHINode *UserPN = dyn_cast(User)) { - if (UserPN->getIncomingBlock(U) == BB || - UserPN->getIncomingBlock(U) == PredBB) + if (UserPN->getIncomingBlock(U) == BB) continue; } else if (User->getParent() == BB) continue; @@ -2013,14 +2008,19 @@ 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()); - // 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); + 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"); } // Ok, NewBB is good to go. Update the terminator of PredBB to jump to @@ -2037,10 +2037,6 @@ bool JumpThreadingPass::ThreadEdge(BasicBlock *BB, {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/lib/Transforms/Utils/SSAUpdaterBulk.cpp b/lib/Transforms/Utils/SSAUpdaterBulk.cpp index 61ceb9c92778..dbc671cc16ff 100644 --- a/lib/Transforms/Utils/SSAUpdaterBulk.cpp +++ b/lib/Transforms/Utils/SSAUpdaterBulk.cpp @@ -38,19 +38,18 @@ static BasicBlock *getUserBB(Use *U) { /// Add a new variable to the SSA rewriter. This needs to be called before /// AddAvailableValue or AddUse calls. -unsigned SSAUpdaterBulk::AddVariable(StringRef Name, Type *Ty) { - unsigned Var = Rewrites.size(); +void SSAUpdaterBulk::AddVariable(unsigned Var, StringRef Name, Type *Ty) { + assert(Rewrites.find(Var) == Rewrites.end() && "Variable added twice!"); DEBUG(dbgs() << "SSAUpdater: Var=" << Var << ": initialized with Ty = " << *Ty << ", Name = " << Name << "\n"); RewriteInfo RI(Name, Ty); - Rewrites.push_back(RI); - return Var; + Rewrites[Var] = RI; } /// 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(Var < Rewrites.size() && "Variable not found!"); + assert(Rewrites.find(Var) != Rewrites.end() && "Should add variable first!"); DEBUG(dbgs() << "SSAUpdater: Var=" << Var << ": added new available value" << *V << " in " << BB->getName() << "\n"); Rewrites[Var].Defines[BB] = V; @@ -59,7 +58,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(Var < Rewrites.size() && "Variable not found!"); + assert(Rewrites.find(Var) != Rewrites.end() && "Should add variable first!"); DEBUG(dbgs() << "SSAUpdater: Var=" << Var << ": added a use" << *U->get() << " in " << getUserBB(U)->getName() << "\n"); Rewrites[Var].Uses.push_back(U); @@ -68,7 +67,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 (Var < Rewrites.size()) ? Rewrites[Var].Defines.count(BB) : false; + return Rewrites.count(Var) ? Rewrites[Var].Defines.count(BB) : false; } // Compute value at the given block BB. We either should already know it, or we @@ -127,14 +126,16 @@ ComputeLiveInBlocks(const SmallPtrSetImpl &UsingBlocks, /// requested uses update. void SSAUpdaterBulk::RewriteAllUses(DominatorTree *DT, SmallVectorImpl *InsertedPHIs) { - for (auto &R : Rewrites) { + for (auto &P : 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); - DEBUG(dbgs() << "SSAUpdater: rewriting " << R.Uses.size() << " use(s)\n"); + RewriteInfo &R = P.second; + DEBUG(dbgs() << "SSAUpdater: Var=" << P.first << ": rewriting " + << R.Uses.size() << " use(s)\n"); SmallPtrSet DefBlocks; for (auto &Def : R.Defines) @@ -164,7 +165,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); @@ -181,8 +182,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: replacing " << *OldVal << " with " << *V - << "\n"); + DEBUG(dbgs() << "SSAUpdater: Var=" << P.first << ": replacing" << *OldVal + << " with " << *V << "\n"); U->set(V); } } diff --git a/test/Transforms/JumpThreading/removed-use.ll b/test/Transforms/JumpThreading/removed-use.ll deleted file mode 100644 index d2c6836fc281..000000000000 --- a/test/Transforms/JumpThreading/removed-use.ll +++ /dev/null @@ -1,28 +0,0 @@ -; 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 -} diff --git a/unittests/Transforms/Utils/SSAUpdaterBulk.cpp b/unittests/Transforms/Utils/SSAUpdaterBulk.cpp index 61cbcb7b1a77..6a8adf9ef37c 100644 --- a/unittests/Transforms/Utils/SSAUpdaterBulk.cpp +++ b/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. - 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)); - - 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)); + 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)); + + 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)); DominatorTree DT(*F); Updater.RewriteAllUses(&DT); @@ -161,19 +161,19 @@ TEST(SSAUpdaterBulk, Irreducible) { // No other rewrites should be made. // Add use in %3. - unsigned VarNum = Updater.AddVariable("c", I32Ty); - Updater.AddAvailableValue(VarNum, IfBB, AddOp1); - Updater.AddUse(VarNum, &I1->getOperandUse(0)); + Updater.AddVariable(0, "c", I32Ty); + Updater.AddAvailableValue(0, IfBB, AddOp1); + Updater.AddUse(0, &I1->getOperandUse(0)); // Add use in %4. - VarNum = Updater.AddVariable("b", I32Ty); - Updater.AddAvailableValue(VarNum, LoopStartBB, AddOp2); - Updater.AddUse(VarNum, &I2->getOperandUse(0)); + Updater.AddVariable(1, "b", I32Ty); + Updater.AddAvailableValue(1, LoopStartBB, AddOp2); + Updater.AddUse(1, &I2->getOperandUse(0)); // Add use in the return instruction. - VarNum = Updater.AddVariable("a", I32Ty); - Updater.AddAvailableValue(VarNum, &F->getEntryBlock(), FirstArg); - Updater.AddUse(VarNum, &Return->getOperandUse(0)); + Updater.AddVariable(2, "a", I32Ty); + Updater.AddAvailableValue(2, &F->getEntryBlock(), FirstArg); + Updater.AddUse(2, &Return->getOperandUse(0)); // Save all inserted phis into a vector. SmallVector Inserted;