[llvm-branch-commits] [llvm] 06d5b1c - [SROA] Remove Dead Instructions while creating speculative instructions
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Fri Dec 18 08:53:41 PST 2020
Author: Arnamoy Bhattacharyya
Date: 2020-12-18T11:47:02-05:00
New Revision: 06d5b1c9ad52476e845f969cf82e00e383f9cf40
URL: https://github.com/llvm/llvm-project/commit/06d5b1c9ad52476e845f969cf82e00e383f9cf40
DIFF: https://github.com/llvm/llvm-project/commit/06d5b1c9ad52476e845f969cf82e00e383f9cf40.diff
LOG: [SROA] Remove Dead Instructions while creating speculative instructions
The SROA pass tries to be lazy for removing dead instructions that are collected during iterative run of the pass in the DeadInsts list. However it does not remove instructions from the dead list while running eraseFromParent() on those instructions.
This causes (rare) null pointer dereferences. For example, in the speculatePHINodeLoads() instruction, in the following code snippet:
```
while (!PN.use_empty()) {
LoadInst *LI = cast<LoadInst>(PN.user_back());
LI->replaceAllUsesWith(NewPN);
LI->eraseFromParent();
}
```
If the Load instruction LI belongs to the DeadInsts list, it should be removed when eraseFromParent() is called. However, the bug does not show up in most cases, because immediately in the same function, a new LoadInst is created in the following line:
```
LoadInst *Load = PredBuilder.CreateAlignedLoad(
LoadTy, InVal, Alignment,
(PN.getName() + ".sroa.speculate.load." + Pred->getName()));
```
This new LoadInst object takes the same memory address of the just deleted LI using eraseFromParent(), therefore the bug does not materialize. In very rare cases, the addresses differ and therefore, a dangling pointer is created, causing a crash.
Reviewed By: lebedev.ri
Differential Revision: https://reviews.llvm.org/D92431
Added:
Modified:
llvm/include/llvm/Transforms/Scalar/SROA.h
llvm/lib/Transforms/Scalar/SROA.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Transforms/Scalar/SROA.h b/llvm/include/llvm/Transforms/Scalar/SROA.h
index 864a0cbd9db1..6ef7c6b22c0b 100644
--- a/llvm/include/llvm/Transforms/Scalar/SROA.h
+++ b/llvm/include/llvm/Transforms/Scalar/SROA.h
@@ -18,6 +18,7 @@
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/IR/PassManager.h"
+#include "llvm/IR/ValueHandle.h"
#include <vector>
namespace llvm {
@@ -77,8 +78,8 @@ class SROA : public PassInfoMixin<SROA> {
/// A collection of instructions to delete.
/// We try to batch deletions to simplify code and make things a bit more
- /// efficient.
- SetVector<Instruction *, SmallVector<Instruction *, 8>> DeadInsts;
+ /// efficient. We also make sure there is no dangling pointers.
+ SmallVector<WeakVH, 8> DeadInsts;
/// Post-promotion worklist.
///
diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index 600863d50d54..7df98694c8b0 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -2460,7 +2460,7 @@ class llvm::sroa::AllocaSliceRewriter
void deleteIfTriviallyDead(Value *V) {
Instruction *I = cast<Instruction>(V);
if (isInstructionTriviallyDead(I))
- Pass.DeadInsts.insert(I);
+ Pass.DeadInsts.push_back(I);
}
Value *rewriteVectorizedLoadInst() {
@@ -2600,7 +2600,7 @@ class llvm::sroa::AllocaSliceRewriter
LI.replaceAllUsesWith(V);
}
- Pass.DeadInsts.insert(&LI);
+ Pass.DeadInsts.push_back(&LI);
deleteIfTriviallyDead(OldOp);
LLVM_DEBUG(dbgs() << " to: " << *V << "\n");
return !LI.isVolatile() && !IsPtrAdjusted;
@@ -2629,7 +2629,7 @@ class llvm::sroa::AllocaSliceRewriter
StoreInst *Store = IRB.CreateAlignedStore(V, &NewAI, NewAI.getAlign());
if (AATags)
Store->setAAMetadata(AATags);
- Pass.DeadInsts.insert(&SI);
+ Pass.DeadInsts.push_back(&SI);
LLVM_DEBUG(dbgs() << " to: " << *Store << "\n");
return true;
@@ -2653,7 +2653,7 @@ class llvm::sroa::AllocaSliceRewriter
LLVMContext::MD_access_group});
if (AATags)
Store->setAAMetadata(AATags);
- Pass.DeadInsts.insert(&SI);
+ Pass.DeadInsts.push_back(&SI);
LLVM_DEBUG(dbgs() << " to: " << *Store << "\n");
return true;
}
@@ -2727,7 +2727,7 @@ class llvm::sroa::AllocaSliceRewriter
NewSI->setAtomic(SI.getOrdering(), SI.getSyncScopeID());
if (NewSI->isAtomic())
NewSI->setAlignment(SI.getAlign());
- Pass.DeadInsts.insert(&SI);
+ Pass.DeadInsts.push_back(&SI);
deleteIfTriviallyDead(OldOp);
LLVM_DEBUG(dbgs() << " to: " << *NewSI << "\n");
@@ -2788,7 +2788,7 @@ class llvm::sroa::AllocaSliceRewriter
}
// Record this instruction for deletion.
- Pass.DeadInsts.insert(&II);
+ Pass.DeadInsts.push_back(&II);
Type *AllocaTy = NewAI.getAllocatedType();
Type *ScalarTy = AllocaTy->getScalarType();
@@ -2958,7 +2958,7 @@ class llvm::sroa::AllocaSliceRewriter
return false;
}
// Record this instruction for deletion.
- Pass.DeadInsts.insert(&II);
+ Pass.DeadInsts.push_back(&II);
// Strip all inbounds GEPs and pointer casts to try to dig out any root
// alloca that should be re-examined after rewriting this instruction.
@@ -3093,7 +3093,7 @@ class llvm::sroa::AllocaSliceRewriter
LLVM_DEBUG(dbgs() << " original: " << II << "\n");
// Record this instruction for deletion.
- Pass.DeadInsts.insert(&II);
+ Pass.DeadInsts.push_back(&II);
if (II.isDroppable()) {
assert(II.getIntrinsicID() == Intrinsic::assume && "Expected assume");
@@ -4076,7 +4076,7 @@ bool SROA::presplitLoadsAndStores(AllocaInst &AI, AllocaSlices &AS) {
}
// Mark the original store as dead.
- DeadInsts.insert(SI);
+ DeadInsts.push_back(SI);
}
// Save the split loads if there are deferred stores among the users.
@@ -4084,7 +4084,7 @@ bool SROA::presplitLoadsAndStores(AllocaInst &AI, AllocaSlices &AS) {
SplitLoadsMap.insert(std::make_pair(LI, std::move(SplitLoads)));
// Mark the original load as dead and kill the original slice.
- DeadInsts.insert(LI);
+ DeadInsts.push_back(LI);
Offsets.S->kill();
}
@@ -4206,9 +4206,9 @@ bool SROA::presplitLoadsAndStores(AllocaInst &AI, AllocaSlices &AS) {
// trivial CSE, including instcombine.
if (LI->hasOneUse()) {
assert(*LI->user_begin() == SI && "Single use isn't this store!");
- DeadInsts.insert(LI);
+ DeadInsts.push_back(LI);
}
- DeadInsts.insert(SI);
+ DeadInsts.push_back(SI);
Offsets.S->kill();
}
@@ -4360,7 +4360,7 @@ AllocaInst *SROA::rewritePartition(AllocaInst &AI, AllocaSlices &AS,
Value::dropDroppableUse(*U);
if (OldInst)
if (isInstructionTriviallyDead(OldInst))
- DeadInsts.insert(OldInst);
+ DeadInsts.push_back(OldInst);
}
if (PHIUsers.empty() && SelectUsers.empty()) {
// Promote the alloca.
@@ -4577,7 +4577,7 @@ void SROA::clobberUse(Use &U) {
// minimal.
if (Instruction *OldI = dyn_cast<Instruction>(OldV))
if (isInstructionTriviallyDead(OldI)) {
- DeadInsts.insert(OldI);
+ DeadInsts.push_back(OldI);
}
}
@@ -4626,7 +4626,7 @@ bool SROA::runOnAlloca(AllocaInst &AI) {
DeadUser->replaceAllUsesWith(UndefValue::get(DeadUser->getType()));
// And mark it for deletion.
- DeadInsts.insert(DeadUser);
+ DeadInsts.push_back(DeadUser);
Changed = true;
}
for (Use *DeadOp : AS.getDeadOperands()) {
@@ -4664,7 +4664,8 @@ bool SROA::deleteDeadInstructions(
SmallPtrSetImpl<AllocaInst *> &DeletedAllocas) {
bool Changed = false;
while (!DeadInsts.empty()) {
- Instruction *I = DeadInsts.pop_back_val();
+ Instruction *I = dyn_cast_or_null<Instruction>(DeadInsts.pop_back_val());
+ if (!I) continue;
LLVM_DEBUG(dbgs() << "Deleting dead instruction: " << *I << "\n");
// If the instruction is an alloca, find the possible dbg.declare connected
@@ -4683,7 +4684,7 @@ bool SROA::deleteDeadInstructions(
// Zero out the operand and see if it becomes trivially dead.
Operand = nullptr;
if (isInstructionTriviallyDead(U))
- DeadInsts.insert(U);
+ DeadInsts.push_back(U);
}
++NumDeleted;
More information about the llvm-branch-commits
mailing list