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

[Jun Bum Lim via llvm-commits]

junbuml 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();
----------------
MatzeB wrote:
> 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)
Thanks for correcting me. 

The easiest way to fix in here would be to store the instructions we want to remove in a separate small vector and then erase all of them separately. If it doesn't make sense, I may see if I can add erase() method in SmallSetVector.
  


http://reviews.llvm.org/D17889