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; _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits