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

[Matthias Braun via llvm-commits]

MatzeB added a comment.

Comparing with http://reviews.llvm.org/D21455 I realized two details:


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

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:284-291
@@ +283,10 @@
+            MachineOperand &PrevCopySrc = PrevCopy.getOperand(1);
+            const TargetRegisterClass *RC =
+              TII->getRegClass(MI->getDesc(), MOIdx, TRI, *MBB.getParent());
+            if (!MRI->isReserved(PrevCopySrc.getReg()) &&
+                !PrevCopySrc.getSubReg() &&
+                RC != nullptr && RC->contains(PrevCopySrc.getReg())) {
+              MO.setReg(PrevCopySrc.getReg());
+              continue;
+            }
+          }
----------------
I found that there are examples where you can only rename some of the operands (the reg may be used multiple times, but on of the inputs is tied to an output and cannot be renamed). It is semantically questionable to rename only parts of the operands (and you will not get any scheduling benefits or dead copy removal opportunities anyway in this case).


http://reviews.llvm.org/D20531