[llvm] r330431 - Revert "Revert r330403 and r330413."
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 20 06:34:33 PDT 2018
Author: mzolotukhin
Date: Fri Apr 20 06:34:32 2018
New Revision: 330431
URL: http://llvm.org/viewvc/llvm-project?rev=330431&view=rev
Log:
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.
Added:
llvm/trunk/test/Transforms/JumpThreading/removed-use.ll
Modified:
llvm/trunk/include/llvm/Transforms/Utils/SSAUpdaterBulk.h
llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp
llvm/trunk/lib/Transforms/Utils/SSAUpdaterBulk.cpp
llvm/trunk/unittests/Transforms/Utils/SSAUpdaterBulk.cpp
Modified: llvm/trunk/include/llvm/Transforms/Utils/SSAUpdaterBulk.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Utils/SSAUpdaterBulk.h?rev=330431&r1=330430&r2=330431&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Transforms/Utils/SSAUpdaterBulk.h (original)
+++ llvm/trunk/include/llvm/Transforms/Utils/SSAUpdaterBulk.h Fri Apr 20 06:34:32 2018
@@ -47,7 +47,7 @@ class SSAUpdaterBulk {
RewriteInfo(){};
RewriteInfo(StringRef &N, Type *T) : Name(N), Ty(T){};
};
- DenseMap<unsigned, RewriteInfo> Rewrites;
+ SmallVector<RewriteInfo, 4> 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.
Modified: llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp?rev=330431&r1=330430&r2=330431&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp Fri Apr 20 06:34:32 2018
@@ -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 <algorithm>
#include <cassert>
@@ -1985,19 +1986,33 @@ bool JumpThreadingPass::ThreadEdge(Basic
// 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<Use*, 16> UsesToRename;
+ SSAUpdaterBulk SSAUpdate;
+
for (Instruction &I : *BB) {
+ SmallVector<Use*, 16> 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<Instruction>(U.getUser());
if (PHINode *UserPN = dyn_cast<PHINode>(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(Basic
// 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");
- }
-
- // 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);
- }
+ // 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);
+ }
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.
Modified: llvm/trunk/lib/Transforms/Utils/SSAUpdaterBulk.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SSAUpdaterBulk.cpp?rev=330431&r1=330430&r2=330431&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Utils/SSAUpdaterBulk.cpp (original)
+++ llvm/trunk/lib/Transforms/Utils/SSAUpdaterBulk.cpp Fri Apr 20 06:34:32 2018
@@ -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(u
/// 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
/// 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 SmallPtrSetImp
/// requested uses update.
void SSAUpdaterBulk::RewriteAllUses(DominatorTree *DT,
SmallVectorImpl<PHINode *> *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<BasicBlock *, 2> DefBlocks;
for (auto &Def : R.Defines)
@@ -165,7 +164,7 @@ void SSAUpdaterBulk::RewriteAllUses(Domi
}
// 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(Domi
// 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);
}
}
Added: llvm/trunk/test/Transforms/JumpThreading/removed-use.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/JumpThreading/removed-use.ll?rev=330431&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/JumpThreading/removed-use.ll (added)
+++ llvm/trunk/test/Transforms/JumpThreading/removed-use.ll Fri Apr 20 06:34:32 2018
@@ -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
+}
Modified: llvm/trunk/unittests/Transforms/Utils/SSAUpdaterBulk.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Transforms/Utils/SSAUpdaterBulk.cpp?rev=330431&r1=330430&r2=330431&view=diff
==============================================================================
--- llvm/trunk/unittests/Transforms/Utils/SSAUpdaterBulk.cpp (original)
+++ llvm/trunk/unittests/Transforms/Utils/SSAUpdaterBulk.cpp Fri Apr 20 06:34:32 2018
@@ -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));
-
- 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));
+ 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));
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<PHINode *, 8> Inserted;
More information about the llvm-commits
mailing list