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

[Matthias Braun via llvm-commits]

MatzeB requested changes to this revision.
MatzeB added a comment.
This revision now requires changes to proceed.

While I also would not expect to hit this often (I am surprised you found an instance in the SPEC at all), I think this change is small enough and very much in the spirit of the existing MachineCopyProp code, so why not go with it? The current patch however is invalid:


================
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();
----------------
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.


http://reviews.llvm.org/D17889