From 06d5b1c9ad52476e845f969cf82e00e383f9cf40 Mon Sep 17 00:00:00 2001 From: Arnamoy Bhattacharyya Date: Fri, 18 Dec 2020 11:38:51 -0500 Subject: [PATCH] [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(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 --- llvm/include/llvm/Transforms/Scalar/SROA.h | 5 +++-- llvm/lib/Transforms/Scalar/SROA.cpp | 35 +++++++++++++++--------------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/llvm/include/llvm/Transforms/Scalar/SROA.h b/llvm/include/llvm/Transforms/Scalar/SROA.h index 864a0cb..6ef7c6b 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 namespace llvm { @@ -77,8 +78,8 @@ class SROA : public PassInfoMixin { /// A collection of instructions to delete. /// We try to batch deletions to simplify code and make things a bit more - /// efficient. - SetVector> DeadInsts; + /// efficient. We also make sure there is no dangling pointers. + SmallVector DeadInsts; /// Post-promotion worklist. /// diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp index 600863d..7df9869 100644 --- a/llvm/lib/Transforms/Scalar/SROA.cpp +++ b/llvm/lib/Transforms/Scalar/SROA.cpp @@ -2460,7 +2460,7 @@ private: void deleteIfTriviallyDead(Value *V) { Instruction *I = cast(V); if (isInstructionTriviallyDead(I)) - Pass.DeadInsts.insert(I); + Pass.DeadInsts.push_back(I); } Value *rewriteVectorizedLoadInst() { @@ -2600,7 +2600,7 @@ private: 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 @@ private: 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 @@ private: 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 @@ private: 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 @@ private: } // 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 @@ private: 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 @@ private: 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(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 &DeletedAllocas) { bool Changed = false; while (!DeadInsts.empty()) { - Instruction *I = DeadInsts.pop_back_val(); + Instruction *I = dyn_cast_or_null(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; -- 2.7.4