[llvm] r278451 - [DSE] Don't remove stores made live by a call which unwinds.
Eli Friedman via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 11 18:09:53 PDT 2016
Author: efriedma
Date: Thu Aug 11 20:09:53 2016
New Revision: 278451
URL: http://llvm.org/viewvc/llvm-project?rev=278451&view=rev
Log:
[DSE] Don't remove stores made live by a call which unwinds.
Issue exposed by noalias or more aggressive alias analysis.
Fixes http://llvm.org/PR25422.
Differential revision: https://reviews.llvm.org/D21007
Modified:
llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp
llvm/trunk/test/Analysis/GlobalsModRef/func-memattributes.ll
llvm/trunk/test/Transforms/DeadStoreElimination/free.ll
llvm/trunk/test/Transforms/DeadStoreElimination/simple.ll
Modified: llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp?rev=278451&r1=278450&r2=278451&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp Thu Aug 11 20:09:53 2016
@@ -70,6 +70,7 @@ static void
deleteDeadInstruction(Instruction *I, BasicBlock::iterator *BBI,
MemoryDependenceResults &MD, const TargetLibraryInfo &TLI,
InstOverlapIntervalsTy &IOL,
+ DenseMap<Instruction*, size_t> *InstrOrdering,
SmallSetVector<Value *, 16> *ValueSet = nullptr) {
SmallVector<Instruction*, 32> NowDeadInsts;
@@ -102,15 +103,14 @@ deleteDeadInstruction(Instruction *I, Ba
NowDeadInsts.push_back(OpI);
}
+ if (ValueSet) ValueSet->remove(DeadInst);
+ InstrOrdering->erase(DeadInst);
+ IOL.erase(DeadInst);
if (NewIter == DeadInst->getIterator())
NewIter = DeadInst->eraseFromParent();
else
DeadInst->eraseFromParent();
-
- IOL.erase(DeadInst);
-
- if (ValueSet) ValueSet->remove(DeadInst);
} while (!NowDeadInsts.empty());
*BBI = NewIter;
}
@@ -590,7 +590,8 @@ static void findUnconditionalPreds(Small
static bool handleFree(CallInst *F, AliasAnalysis *AA,
MemoryDependenceResults *MD, DominatorTree *DT,
const TargetLibraryInfo *TLI,
- InstOverlapIntervalsTy &IOL) {
+ InstOverlapIntervalsTy &IOL,
+ DenseMap<Instruction*, size_t> *InstrOrdering) {
bool MadeChange = false;
MemoryLocation Loc = MemoryLocation(F->getOperand(0));
@@ -622,7 +623,7 @@ static bool handleFree(CallInst *F, Alia
// DCE instructions only used to calculate that store.
BasicBlock::iterator BBI(Dependency);
- deleteDeadInstruction(Dependency, &BBI, *MD, *TLI, IOL);
+ deleteDeadInstruction(Dependency, &BBI, *MD, *TLI, IOL, InstrOrdering);
++NumFastStores;
MadeChange = true;
@@ -678,7 +679,8 @@ static void removeAccessedObjects(const
static bool handleEndBlock(BasicBlock &BB, AliasAnalysis *AA,
MemoryDependenceResults *MD,
const TargetLibraryInfo *TLI,
- InstOverlapIntervalsTy &IOL) {
+ InstOverlapIntervalsTy &IOL,
+ DenseMap<Instruction*, size_t> *InstrOrdering) {
bool MadeChange = false;
// Keep track of all of the stack objects that are dead at the end of the
@@ -737,7 +739,7 @@ static bool handleEndBlock(BasicBlock &B
dbgs() << '\n');
// DCE instructions only used to calculate that store.
- deleteDeadInstruction(Dead, &BBI, *MD, *TLI, IOL, &DeadStackObjects);
+ deleteDeadInstruction(Dead, &BBI, *MD, *TLI, IOL, InstrOrdering, &DeadStackObjects);
++NumFastStores;
MadeChange = true;
continue;
@@ -748,7 +750,7 @@ static bool handleEndBlock(BasicBlock &B
if (isInstructionTriviallyDead(&*BBI, TLI)) {
DEBUG(dbgs() << "DSE: Removing trivially dead instruction:\n DEAD: "
<< *&*BBI << '\n');
- deleteDeadInstruction(&*BBI, &BBI, *MD, *TLI, IOL, &DeadStackObjects);
+ deleteDeadInstruction(&*BBI, &BBI, *MD, *TLI, IOL, InstrOrdering, &DeadStackObjects);
++NumFastOther;
MadeChange = true;
continue;
@@ -947,7 +949,8 @@ static bool eliminateNoopStore(Instructi
AliasAnalysis *AA, MemoryDependenceResults *MD,
const DataLayout &DL,
const TargetLibraryInfo *TLI,
- InstOverlapIntervalsTy &IOL) {
+ InstOverlapIntervalsTy &IOL,
+ DenseMap<Instruction*, size_t> *InstrOrdering) {
// Must be a store instruction.
StoreInst *SI = dyn_cast<StoreInst>(Inst);
if (!SI)
@@ -962,7 +965,7 @@ static bool eliminateNoopStore(Instructi
DEBUG(dbgs() << "DSE: Remove Store Of Load from same pointer:\n LOAD: "
<< *DepLoad << "\n STORE: " << *SI << '\n');
- deleteDeadInstruction(SI, &BBI, *MD, *TLI, IOL);
+ deleteDeadInstruction(SI, &BBI, *MD, *TLI, IOL, InstrOrdering);
++NumRedundantStores;
return true;
}
@@ -980,7 +983,7 @@ static bool eliminateNoopStore(Instructi
dbgs() << "DSE: Remove null store to the calloc'ed object:\n DEAD: "
<< *Inst << "\n OBJECT: " << *UnderlyingPointer << '\n');
- deleteDeadInstruction(SI, &BBI, *MD, *TLI, IOL);
+ deleteDeadInstruction(SI, &BBI, *MD, *TLI, IOL, InstrOrdering);
++NumRedundantStores;
return true;
}
@@ -994,6 +997,12 @@ static bool eliminateDeadStores(BasicBlo
const DataLayout &DL = BB.getModule()->getDataLayout();
bool MadeChange = false;
+ // FIXME: Maybe change this to use some abstraction like OrderedBasicBlock?
+ // The current OrderedBasicBlock can't deal with mutation at the moment.
+ size_t LastThrowingInstIndex = 0;
+ DenseMap<Instruction*, size_t> InstrOrdering;
+ size_t InstrIndex = 1;
+
// A map of interval maps representing partially-overwritten value parts.
InstOverlapIntervalsTy IOL;
@@ -1001,7 +1010,7 @@ static bool eliminateDeadStores(BasicBlo
for (BasicBlock::iterator BBI = BB.begin(), BBE = BB.end(); BBI != BBE; ) {
// Handle 'free' calls specially.
if (CallInst *F = isFreeCall(&*BBI, TLI)) {
- MadeChange |= handleFree(F, AA, MD, DT, TLI, IOL);
+ MadeChange |= handleFree(F, AA, MD, DT, TLI, IOL, &InstrOrdering);
// Increment BBI after handleFree has potentially deleted instructions.
// This ensures we maintain a valid iterator.
++BBI;
@@ -1010,12 +1019,19 @@ static bool eliminateDeadStores(BasicBlo
Instruction *Inst = &*BBI++;
+ size_t CurInstNumber = InstrIndex++;
+ InstrOrdering.insert(std::make_pair(Inst, CurInstNumber));
+ if (Inst->mayThrow()) {
+ LastThrowingInstIndex = CurInstNumber;
+ continue;
+ }
+
// Check to see if Inst writes to memory. If not, continue.
if (!hasMemoryWrite(Inst, *TLI))
continue;
// eliminateNoopStore will update in iterator, if necessary.
- if (eliminateNoopStore(Inst, BBI, AA, MD, DL, TLI, IOL)) {
+ if (eliminateNoopStore(Inst, BBI, AA, MD, DL, TLI, IOL, &InstrOrdering)) {
MadeChange = true;
continue;
}
@@ -1049,6 +1065,31 @@ static bool eliminateDeadStores(BasicBlo
if (!DepLoc.Ptr)
break;
+ // Make sure we don't look past a call which might throw. This is an
+ // issue because MemoryDependenceAnalysis works in the wrong direction:
+ // it finds instructions which dominate the current instruction, rather than
+ // instructions which are post-dominated by the current instruction.
+ //
+ // If the underlying object is a non-escaping memory allocation, any store
+ // to it is dead along the unwind edge. Otherwise, we need to preserve
+ // the store.
+ size_t DepIndex = InstrOrdering.lookup(DepWrite);
+ assert(DepIndex && "Unexpected instruction");
+ if (DepIndex <= LastThrowingInstIndex) {
+ const Value* Underlying = GetUnderlyingObject(DepLoc.Ptr, DL);
+ bool IsStoreDeadOnUnwind = isa<AllocaInst>(Underlying);
+ if (!IsStoreDeadOnUnwind) {
+ // We're looking for a call to an allocation function
+ // where the allocation doesn't escape before the last
+ // throwing instruction; PointerMayBeCaptured
+ // reasonably fast approximation.
+ IsStoreDeadOnUnwind = isAllocLikeFn(Underlying, TLI) &&
+ !PointerMayBeCaptured(Underlying, false, true);
+ }
+ if (!IsStoreDeadOnUnwind)
+ break;
+ }
+
// If we find a write that is a) removable (i.e., non-volatile), b) is
// completely obliterated by the store to 'Loc', and c) which we know that
// 'Inst' doesn't load from, then we can remove it.
@@ -1063,7 +1104,7 @@ static bool eliminateDeadStores(BasicBlo
<< *DepWrite << "\n KILLER: " << *Inst << '\n');
// Delete the store and now-dead instructions that feed it.
- deleteDeadInstruction(DepWrite, &BBI, *MD, *TLI, IOL);
+ deleteDeadInstruction(DepWrite, &BBI, *MD, *TLI, IOL, &InstrOrdering);
++NumFastStores;
MadeChange = true;
@@ -1109,7 +1150,7 @@ static bool eliminateDeadStores(BasicBlo
// If this block ends in a return, unwind, or unreachable, all allocas are
// dead at its end, which means stores to them are also dead.
if (BB.getTerminator()->getNumSuccessors() == 0)
- MadeChange |= handleEndBlock(BB, AA, MD, TLI, IOL);
+ MadeChange |= handleEndBlock(BB, AA, MD, TLI, IOL, &InstrOrdering);
return MadeChange;
}
@@ -1123,6 +1164,7 @@ static bool eliminateDeadStores(Function
// cycles that will confuse alias analysis.
if (DT->isReachableFromEntry(&BB))
MadeChange |= eliminateDeadStores(BB, AA, MD, DT, TLI);
+
return MadeChange;
}
Modified: llvm/trunk/test/Analysis/GlobalsModRef/func-memattributes.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/GlobalsModRef/func-memattributes.ll?rev=278451&r1=278450&r2=278451&view=diff
==============================================================================
--- llvm/trunk/test/Analysis/GlobalsModRef/func-memattributes.ll (original)
+++ llvm/trunk/test/Analysis/GlobalsModRef/func-memattributes.ll Thu Aug 11 20:09:53 2016
@@ -2,30 +2,30 @@
@X = internal global i32 4
-define void @test0() {
+define i32 @test0() {
; CHECK-LABEL: @test0
; CHECK: store i32 0, i32* @X
-; CHECK-NEXT: call void @func_readonly() #0
+; CHECK-NEXT: call i32 @func_readonly() #0
; CHECK-NEXT: store i32 1, i32* @X
store i32 0, i32* @X
- call void @func_readonly() #0
+ %x = call i32 @func_readonly() #0
store i32 1, i32* @X
- ret void
+ ret i32 %x
}
-define void @test1() {
+define i32 @test1() {
; CHECK-LABEL: @test1
; CHECK-NOT: store
-; CHECK: call void @func_read_argmem_only() #1
+; CHECK: call i32 @func_read_argmem_only() #1
; CHECK-NEXT: store i32 3, i32* @X
store i32 2, i32* @X
- call void @func_read_argmem_only() #1
+ %x = call i32 @func_read_argmem_only() #1
store i32 3, i32* @X
- ret void
+ ret i32 %x
}
-declare void @func_readonly() #0
-declare void @func_read_argmem_only() #1
+declare i32 @func_readonly() #0
+declare i32 @func_read_argmem_only() #1
-attributes #0 = { readonly }
-attributes #1 = { readonly argmemonly }
+attributes #0 = { readonly nounwind }
+attributes #1 = { readonly argmemonly nounwind }
Modified: llvm/trunk/test/Transforms/DeadStoreElimination/free.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadStoreElimination/free.ll?rev=278451&r1=278450&r2=278451&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/DeadStoreElimination/free.ll (original)
+++ llvm/trunk/test/Transforms/DeadStoreElimination/free.ll Thu Aug 11 20:09:53 2016
@@ -13,7 +13,7 @@ define void @test(i32* %Q, i32* %P) {
%DEAD = load i32, i32* %Q ; <i32> [#uses=1]
store i32 %DEAD, i32* %P
%1 = bitcast i32* %P to i8*
- tail call void @free(i8* %1)
+ tail call void @free(i8* %1) nounwind
ret void
}
@@ -25,7 +25,7 @@ define void @test2({i32, i32}* %P) {
%Q = getelementptr {i32, i32}, {i32, i32} *%P, i32 0, i32 1
store i32 4, i32* %Q
%1 = bitcast {i32, i32}* %P to i8*
- tail call void @free(i8* %1)
+ tail call void @free(i8* %1) nounwind
ret void
}
@@ -37,7 +37,7 @@ define void @test3() {
store i8 0, i8* %m
%m1 = getelementptr i8, i8* %m, i64 1
store i8 1, i8* %m1
- call void @free(i8* %m)
+ call void @free(i8* %m) nounwind
ret void
}
Modified: llvm/trunk/test/Transforms/DeadStoreElimination/simple.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadStoreElimination/simple.ll?rev=278451&r1=278450&r2=278451&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/DeadStoreElimination/simple.ll (original)
+++ llvm/trunk/test/Transforms/DeadStoreElimination/simple.ll Thu Aug 11 20:09:53 2016
@@ -498,3 +498,26 @@ bb3:
ret i32 0
}
+; Don't remove redundant store: unknown_func could unwind
+; CHECK-LABEL: @test34(
+; CHECK: store i32 1
+; CHECK: store i32 0
+; CHECK: ret
+define void @test34(i32* noalias %p) {
+ store i32 1, i32* %p
+ call void @unknown_func()
+ store i32 0, i32* %p
+ ret void
+}
+
+; Remove redundant store even with an unwinding function in the same block
+; CHECK-LABEL: @test35(
+; CHECK: call void @unknown_func
+; CHECK-NEXT: store i32 0
+; CHECK-NEXT: ret void
+define void @test35(i32* noalias %p) {
+ call void @unknown_func()
+ store i32 1, i32* %p
+ store i32 0, i32* %p
+ ret void
+}
More information about the llvm-commits
mailing list