Skip to content

Commit

Permalink
Fix for a dangling point bug in DeadStoreElimination pass
Browse files Browse the repository at this point in the history 
The patch makes sure that the LastThrowing pointer does not point to any instruction deleted by call to DeleteDeadInstruction.

While iterating through the instructions the pass maintains a pointer to the lastThrowing Instruction. A call to deleteDeadInstruction deletes a dead store and other instructions feeding the original dead instruction which also become dead. The instruction pointed by the lastThrowing pointer could also be deleted by the call to DeleteDeadInstruction and thus it becomes a dangling pointer. Because of this, we see an error in the next iteration.

In the patch, we maintain a list of throwing instructions encountered previously and use the last non deleted throwing instruction from the container.

Reviewers: fhahn, bcahoon, efriedma

Reviewed By: fhahn

Differential Revision: https://reviews.llvm.org/D65326
  • Loading branch information
@aankit-ca @fhahn
aankit-ca authored and fhahn committed Jan 3, 2020
1 parent 1640582 commit 369a919
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 17 deletions.
56 changes: 39 additions & 17 deletions llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "llvm/Transforms/Scalar/DeadStoreElimination.h"
#include "llvm/ADT/APInt.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/MapVector.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
Expand Down Expand Up @@ -100,6 +101,7 @@ static void
deleteDeadInstruction(Instruction *I, BasicBlock::iterator *BBI,
MemoryDependenceResults &MD, const TargetLibraryInfo &TLI,
InstOverlapIntervalsTy &IOL, OrderedBasicBlock &OBB,
MapVector<Instruction *, bool> &ThrowableInst,
SmallSetVector<const Value *, 16> *ValueSet = nullptr) {
SmallVector<Instruction*, 32> NowDeadInsts;

Expand All @@ -113,6 +115,10 @@ deleteDeadInstruction(Instruction *I, BasicBlock::iterator *BBI,
// Before we touch this instruction, remove it from memdep!
do {
Instruction *DeadInst = NowDeadInsts.pop_back_val();
// Mark the DeadInst as dead in the list of throwable instructions.
auto It = ThrowableInst.find(DeadInst);
if (It != ThrowableInst.end())
ThrowableInst[It->first] = false;
++NumFastOther;

// Try to preserve debug information attached to the dead instruction.
Expand Down Expand Up @@ -145,6 +151,9 @@ deleteDeadInstruction(Instruction *I, BasicBlock::iterator *BBI,
DeadInst->eraseFromParent();
} while (!NowDeadInsts.empty());
*BBI = NewIter;
// Pop dead entries from back of ThrowableInst till we find an alive entry.
while (!ThrowableInst.empty() && !ThrowableInst.back().second)
ThrowableInst.pop_back();
}

/// Does this instruction write some memory? This only returns true for things
Expand Down Expand Up @@ -657,7 +666,8 @@ static void findUnconditionalPreds(SmallVectorImpl<BasicBlock *> &Blocks,
static bool handleFree(CallInst *F, AliasAnalysis *AA,
MemoryDependenceResults *MD, DominatorTree *DT,
const TargetLibraryInfo *TLI,
InstOverlapIntervalsTy &IOL, OrderedBasicBlock &OBB) {
InstOverlapIntervalsTy &IOL, OrderedBasicBlock &OBB,
MapVector<Instruction *, bool> &ThrowableInst) {
bool MadeChange = false;

MemoryLocation Loc = MemoryLocation(F->getOperand(0));
Expand Down Expand Up @@ -691,7 +701,8 @@ static bool handleFree(CallInst *F, AliasAnalysis *AA,

// DCE instructions only used to calculate that store.
BasicBlock::iterator BBI(Dependency);
deleteDeadInstruction(Dependency, &BBI, *MD, *TLI, IOL, OBB);
deleteDeadInstruction(Dependency, &BBI, *MD, *TLI, IOL, OBB,
ThrowableInst);
++NumFastStores;
MadeChange = true;

Expand Down Expand Up @@ -748,8 +759,8 @@ static void removeAccessedObjects(const MemoryLocation &LoadedLoc,
static bool handleEndBlock(BasicBlock &BB, AliasAnalysis *AA,
MemoryDependenceResults *MD,
const TargetLibraryInfo *TLI,
InstOverlapIntervalsTy &IOL,
OrderedBasicBlock &OBB) {
InstOverlapIntervalsTy &IOL, OrderedBasicBlock &OBB,
MapVector<Instruction *, bool> &ThrowableInst) {
bool MadeChange = false;

// Keep track of all of the stack objects that are dead at the end of the
Expand Down Expand Up @@ -810,7 +821,7 @@ static bool handleEndBlock(BasicBlock &BB, AliasAnalysis *AA,
<< '\n');

// DCE instructions only used to calculate that store.
deleteDeadInstruction(Dead, &BBI, *MD, *TLI, IOL, OBB,
deleteDeadInstruction(Dead, &BBI, *MD, *TLI, IOL, OBB, ThrowableInst,
&DeadStackObjects);
++NumFastStores;
MadeChange = true;
Expand All @@ -822,7 +833,7 @@ static bool handleEndBlock(BasicBlock &BB, AliasAnalysis *AA,
if (isInstructionTriviallyDead(&*BBI, TLI)) {
LLVM_DEBUG(dbgs() << "DSE: Removing trivially dead instruction:\n DEAD: "
<< *&*BBI << '\n');
deleteDeadInstruction(&*BBI, &BBI, *MD, *TLI, IOL, OBB,
deleteDeadInstruction(&*BBI, &BBI, *MD, *TLI, IOL, OBB, ThrowableInst,
&DeadStackObjects);
++NumFastOther;
MadeChange = true;
Expand Down Expand Up @@ -1029,7 +1040,8 @@ static bool eliminateNoopStore(Instruction *Inst, BasicBlock::iterator &BBI,
const DataLayout &DL,
const TargetLibraryInfo *TLI,
InstOverlapIntervalsTy &IOL,
OrderedBasicBlock &OBB) {
OrderedBasicBlock &OBB,
MapVector<Instruction *, bool> &ThrowableInst) {
// Must be a store instruction.
StoreInst *SI = dyn_cast<StoreInst>(Inst);
if (!SI)
Expand All @@ -1045,7 +1057,7 @@ static bool eliminateNoopStore(Instruction *Inst, BasicBlock::iterator &BBI,
dbgs() << "DSE: Remove Store Of Load from same pointer:\n LOAD: "
<< *DepLoad << "\n STORE: " << *SI << '\n');

deleteDeadInstruction(SI, &BBI, *MD, *TLI, IOL, OBB);
deleteDeadInstruction(SI, &BBI, *MD, *TLI, IOL, OBB, ThrowableInst);
++NumRedundantStores;
return true;
}
Expand All @@ -1063,7 +1075,7 @@ static bool eliminateNoopStore(Instruction *Inst, BasicBlock::iterator &BBI,
dbgs() << "DSE: Remove null store to the calloc'ed object:\n DEAD: "
<< *Inst << "\n OBJECT: " << *UnderlyingPointer << '\n');

deleteDeadInstruction(SI, &BBI, *MD, *TLI, IOL, OBB);
deleteDeadInstruction(SI, &BBI, *MD, *TLI, IOL, OBB, ThrowableInst);
++NumRedundantStores;
return true;
}
Expand All @@ -1078,7 +1090,7 @@ static bool eliminateDeadStores(BasicBlock &BB, AliasAnalysis *AA,
bool MadeChange = false;

OrderedBasicBlock OBB(&BB);
Instruction *LastThrowing = nullptr;
MapVector<Instruction *, bool> ThrowableInst;

// A map of interval maps representing partially-overwritten value parts.
InstOverlapIntervalsTy IOL;
Expand All @@ -1087,7 +1099,7 @@ static bool eliminateDeadStores(BasicBlock &BB, AliasAnalysis *AA,
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, OBB);
MadeChange |= handleFree(F, AA, MD, DT, TLI, IOL, OBB, ThrowableInst);
// Increment BBI after handleFree has potentially deleted instructions.
// This ensures we maintain a valid iterator.
++BBI;
Expand All @@ -1097,7 +1109,7 @@ static bool eliminateDeadStores(BasicBlock &BB, AliasAnalysis *AA,
Instruction *Inst = &*BBI++;

if (Inst->mayThrow()) {
LastThrowing = Inst;
ThrowableInst[Inst] = true;
continue;
}

Expand All @@ -1106,7 +1118,8 @@ static bool eliminateDeadStores(BasicBlock &BB, AliasAnalysis *AA,
continue;

// eliminateNoopStore will update in iterator, if necessary.
if (eliminateNoopStore(Inst, BBI, AA, MD, DL, TLI, IOL, OBB)) {
if (eliminateNoopStore(Inst, BBI, AA, MD, DL, TLI, IOL, OBB,
ThrowableInst)) {
MadeChange = true;
continue;
}
Expand Down Expand Up @@ -1149,6 +1162,12 @@ static bool eliminateDeadStores(BasicBlock &BB, AliasAnalysis *AA,
if (!DepLoc.Ptr)
break;

// Find the last throwable instruction not removed by call to
// deleteDeadInstruction.
Instruction *LastThrowing = nullptr;
if (!ThrowableInst.empty())
LastThrowing = ThrowableInst.back().first;

// 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
Expand Down Expand Up @@ -1188,7 +1207,8 @@ static bool eliminateDeadStores(BasicBlock &BB, AliasAnalysis *AA,
<< "\n KILLER: " << *Inst << '\n');

// Delete the store and now-dead instructions that feed it.
deleteDeadInstruction(DepWrite, &BBI, *MD, *TLI, IOL, OBB);
deleteDeadInstruction(DepWrite, &BBI, *MD, *TLI, IOL, OBB,
ThrowableInst);
++NumFastStores;
MadeChange = true;

Expand Down Expand Up @@ -1270,8 +1290,10 @@ static bool eliminateDeadStores(BasicBlock &BB, AliasAnalysis *AA,
OBB.replaceInstruction(DepWrite, SI);

// Delete the old stores and now-dead instructions that feed them.
deleteDeadInstruction(Inst, &BBI, *MD, *TLI, IOL, OBB);
deleteDeadInstruction(DepWrite, &BBI, *MD, *TLI, IOL, OBB);
deleteDeadInstruction(Inst, &BBI, *MD, *TLI, IOL, OBB,
ThrowableInst);
deleteDeadInstruction(DepWrite, &BBI, *MD, *TLI, IOL, OBB,
ThrowableInst);
MadeChange = true;

// We erased DepWrite and Inst (Loc); start over.
Expand Down Expand Up @@ -1306,7 +1328,7 @@ static bool eliminateDeadStores(BasicBlock &BB, AliasAnalysis *AA,
// 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, OBB);
MadeChange |= handleEndBlock(BB, AA, MD, TLI, IOL, OBB, ThrowableInst);

return MadeChange;
}
Expand Down
41 changes: 41 additions & 0 deletions llvm/test/Transforms/DeadStoreElimination/DeleteThrowableInst.ll
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt < %s -basicaa -dse -S | FileCheck %s

declare i8* @_Znwj(i32) local_unnamed_addr
declare void @foo() readnone

define void @test1(i8** %ptr) {
; CHECK-LABEL: @test1(
; CHECK-NEXT: [[VAL:%.*]] = inttoptr i64 23452 to i8*
; CHECK-NEXT: store i8* [[VAL]], i8** [[PTR:%.*]]
; CHECK-NEXT: ret void
;
%val = inttoptr i64 23452 to i8*
store i8* %val, i8** %ptr
%call = call i8* @_Znwj(i32 1)
store i8* %call, i8** %ptr
store i8* %val, i8** %ptr
ret void
}

define void @test2(i8** %ptr, i8* %p1, i8* %p2) {
; CHECK-LABEL: @test2(
; CHECK-NEXT: [[VAL:%.*]] = inttoptr i64 23452 to i8*
; CHECK-NEXT: store i8* [[VAL]], i8** [[PTR:%.*]]
; CHECK-NEXT: call void @foo()
; CHECK-NEXT: store i8* [[P1:%.*]], i8** [[PTR]]
; CHECK-NEXT: call void @foo()
; CHECK-NEXT: store i8* [[VAL]], i8** [[PTR]]
; CHECK-NEXT: ret void
;
%val = inttoptr i64 23452 to i8*
store i8* %val, i8** %ptr
call void @foo()
store i8* %p1, i8** %ptr
call void @foo()
store i8* %p2, i8** %ptr
%call = call i8* @_Znwj(i32 1)
store i8* %call, i8** %ptr
store i8* %val, i8** %ptr
ret void
}

0 comments on commit 369a919

Please sign in to comment.