[PATCH] D20531: MachineCopyPropagation: remove some more copys when they are not needed

[Jonas Paulsson via llvm-commits]

jonpa added inline comments.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:234-236
@@ -231,2 +233,5 @@
       ClobberRegister(Def);
+      // Handle any second implicit def operand.
+      if (MI->getNumOperands() == 3 && MI->getOperand(2).isDef())
+        ClobberRegister(MI->getOperand(2).getReg());
 
----------------
MatzeB wrote:
> This looks strange. In theory we can see any number of additional implicit operands on a COPY. I assume we were just to not hit any issues because in todays practice we only add implicit defs and uses of the superregister when a subregister was used (see code in VirtRegRewriter).
> 
> Do you actually need this bugfix as part of this patch? As far as I can see COPY instructions are not renamed here so we should not run into any new problems here.
> 
> (It would still be good to upstream this fix in a separate commit through and use a loop so we also catch the case with 4 operands when we had a subregister def and use at the COPY).
I thought that it was kind of a bug to not ignore an implicit-def operand regardless of anything else.

As I recall, it was also necessary to handle during my experiments, either because the MachineVerifier complained that a register was used without a def (if that COPY was removed), or because I wanted to avoid bothering with subregisters all together (changing just one subregister is not a good idea). Not sure exactly which of these I ran into.


http://reviews.llvm.org/D20531