SROA: Remove pointer from visited along with instruction

[rampitec]

Summary

If an instruction is erased we also need to remove it from
Visited set. There is a very small chance that an another
newly created instruction will be created with the same
pointer value in place of an erased one.

[arsenm]

Is this a new problem from the recent patches or was this already broken?

[rampitec]

In D80958#2076232, @arsenm wrote:

Is this a new problem from the recent patches or was this already broken?

It's been there for two instances and I have added two more recently. It seems to not really happen or at least not too often, but anyway there is a possibility.

llvm/lib/Transforms/Scalar/SROA.cpp

bool visitLoadInst(LoadInst &LI) {
     // We have an aggregate being loaded, split it apart.
     LLVM_DEBUG(dbgs() << "    original: " << LI << "\n");
     AAMDNodes AATags;
     LI.getAAMetadata(AATags);
     LoadOpSplitter Splitter(&LI, *U, LI.getType(), AATags,
                             getAdjustedAlignment(&LI, 0), DL);
     Value *V = UndefValue::get(LI.getType());
     Splitter.emitSplitOps(LI.getType(), V, LI.getName() + ".fca");
+    Visited.erase(&LI);
     LI.replaceAllUsesWith(V);
     LI.eraseFromParent();
     return true;
   }
 
   struct StoreOpSplitter : public OpSplitter<StoreOpSplitter> {
     StoreOpSplitter(Instruction *InsertionPoint, Value *Ptr, Type *BaseTy,
                     AAMDNodes AATags, Align BaseAlign, const DataLayout &DL)
bool visitStoreInst(StoreInst &SI) {
 
     // We have an aggregate being stored, split it apart.
     LLVM_DEBUG(dbgs() << "    original: " << SI << "\n");
     AAMDNodes AATags;
     SI.getAAMetadata(AATags);
     StoreOpSplitter Splitter(&SI, *U, V->getType(), AATags,
                              getAdjustedAlignment(&SI, 0), DL);
     Splitter.emitSplitOps(V->getType(), V, V->getName() + ".fca");
+    Visited.erase(&SI);
     SI.eraseFromParent();
     return true;
   }
 
   bool visitBitCastInst(BitCastInst &BC) {
     enqueueUsers(BC);
     return false;
   }
bool foldGEPSelect(GetElementPtrInst &GEPI) {
     Value *NFalse =
         IsInBounds
             ? Builder.CreateInBoundsGEP(False, Index,
                                         False->getName() + ".sroa.gep")
             : Builder.CreateGEP(False, Index, False->getName() + ".sroa.gep");
 
     Value *NSel = Builder.CreateSelect(Sel->getCondition(), NTrue, NFalse,
                                        Sel->getName() + ".sroa.sel");
+    Visited.erase(&GEPI);
     GEPI.replaceAllUsesWith(NSel);
     GEPI.eraseFromParent();
     Instruction *NSelI = cast<Instruction>(NSel);
     Visited.insert(NSelI);
     enqueueUsers(*NSelI);
 
     LLVM_DEBUG(dbgs() << "\n          to: " << *NTrue
                       << "\n              " << *NFalse
for (unsigned I = 0, E = PHI->getNumIncomingValues(); I != E; ++I) {
 
       IRBuilderTy B(In->getParent(), std::next(In->getIterator()));
       Value *NewVal = IsInBounds
           ? B.CreateInBoundsGEP(In, Index, In->getName() + ".sroa.gep")
           : B.CreateGEP(In, Index, In->getName() + ".sroa.gep");
       NewPN->addIncoming(NewVal, PHI->getIncomingBlock(I));
     }
 
+    Visited.erase(&GEPI);
     GEPI.replaceAllUsesWith(NewPN);
     GEPI.eraseFromParent();
     Visited.insert(NewPN);
     enqueueUsers(*NewPN);
 
     LLVM_DEBUG(for (Value *In : NewPN->incoming_values())
                  dbgs() << "\n              " << *In;
                dbgs() << "\n              " << *NewPN << '\n');