[PATCH] D17889: [MachineCopyPropagation] Expose more dead copies across instructions with regmasks

[Matthias Braun via llvm-commits]

MatzeB added inline comments.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:288-301
@@ -287,9 +287,16 @@
       // Erase any MaybeDeadCopies whose destination register is clobbered.
-      for (MachineInstr *MaybeDead : MaybeDeadCopies) {
+      for (SmallSetVector<MachineInstr *, 8>::iterator
+               DI = MaybeDeadCopies.begin(),
+               DE = MaybeDeadCopies.end();
+           DI != DE;) {
+        MachineInstr *MaybeDead = *DI;
+        ++DI;
+
         unsigned Reg = MaybeDead->getOperand(0).getReg();
         assert(!MRI->isReserved(Reg));
         if (!RegMask->clobbersPhysReg(Reg))
           continue;
         DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: ";
               MaybeDead->dump());
+        MaybeDeadCopies.remove(MaybeDead);
         MaybeDead->eraseFromParent();
----------------
junbuml wrote:
> MatzeB wrote:
> > It seems to me like SmallSetVector::remove() invalidates all iterators into the structure (as it removes elements from an underlying vector and the iterators are just iterators into that vector). May be a good idea to add a SmallSetVector erase(iterator) method that gives returns you a valid iterator similar to SmallVector::erase() or we need a way to deal with it here.
> For me, it seems fine to use remove(), which erase in underlying set_ first and then in vector_, since we actually iterate using vector_ and increase our iterator (DI) right after assigning the current iterator to MaybeDead. 
increasing the iterator first and then removing works fine for llvm::DenseMap, llvm::SmallPtrSet, etc. but not for vectors!
I rechecked this: std::vector::erase() invalidates all iterators that point to the removed element or behind it including the end() iterator. llvm::SmallVector should behave the same in that respect.

(Just think about a typical vector implementation, iterators are just pointers into the array when you remove elements everything behind will be moved forward, a loop like above will make you miss the element after the one you removed in case of the end you will even wander off into memory behind the vector)


http://reviews.llvm.org/D17889