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

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:288-302
@@ -287,10 +287,17 @@
       // Erase any MaybeDeadCopies whose destination register is clobbered.
-      for (MachineInstr *MaybeDead : MaybeDeadCopies) {
+      for (SmallSetVector<MachineInstr *, 8>::iterator DI =
+               MaybeDeadCopies.begin();
+           DI != MaybeDeadCopies.end();) {
+        MachineInstr *MaybeDead = *DI;
         unsigned Reg = MaybeDead->getOperand(0).getReg();
         assert(!MRI->isReserved(Reg));
-        if (!RegMask->clobbersPhysReg(Reg))
+
+        if (!RegMask->clobbersPhysReg(Reg)) {
+          ++DI;
           continue;
+        }
         DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: ";
               MaybeDead->dump());
+        MaybeDeadCopies.remove(MaybeDead);
         MaybeDead->eraseFromParent();
         Changed = true;
----------------
junbuml wrote:
> qcolombet wrote:
> > junbuml wrote:
> > > Modified the way I iterate the loop; do not increase the iterator when removing an element. 
> > This is not clear to me how the iterator makes progress when we remove an element now.
> > Could you explain what I am missing, please?
> Erasing an element will causes the vector to relocate all the elements after the erased one to their new positions; like one element shift left. So the current iterator will point the right next element after erasing.
> 
Is this something the API guarantee?
I was under the impression that although you are right about this behavior, we are actually abusing it by taking advantage of undocumented feature.

Put differently we shouldn’t rely on that behavior unless the API guarantees it. I think this is what Matthias wanted you to go with the additional erase method, i.e., a method what does what you want and is properly documented.


http://reviews.llvm.org/D17889