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

[Jun Bum Lim via llvm-commits]

junbuml added a comment.

>From my test with spec, the dead copies are just exposed in this pass. In the code below, the second copy is removed as a NOP copy after then the first copy remain just as a dead copy because we currently clear MaybeDeadCopies when encountering an instruction with regmask.

  %X19<def> = COPY %X0
  %X0<def> = COPY %X19<kill>
  BL <ga:@_Unwind_Resume>, %X0<imp-use>

I think removing only the instruction erased is just the right way to follow the original intention of MaybeDeadCopies.


================
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:
> 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. 


http://reviews.llvm.org/D17889