[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-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;
----------------
qcolombet wrote:
> 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.
Okay, I see your point. To be clear let me add a new erase method in SmallSetVector which will return the next element we need to iterate after erasing. 

Thanks Quentin for revisiting this. 



http://reviews.llvm.org/D17889