[PATCH] D17475: MachineCopyPropagation: Catch copies of the form A<-B; A<-B

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:55
@@ -54,2 +54,3 @@
     void CopyPropagateBlock(MachineBasicBlock &MBB);
+    bool eraseRedundantCopy(MachineInstr *MI, unsigned Src, unsigned Def);
 
----------------
Please document this function.
Although based on the uses, I found Src and Def being confusing because they do not actually represent the source and destination of MI.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:209
@@ +208,3 @@
+      // %RAX<def> = COPY %RSP
+      if (eraseRedundantCopy(MI, Def, Src))
+        continue;
----------------
Unless I am missing something, the previous comment is a plain copy of the previous one, whereas what you want is:
      //  %ECX<def> = COPY %EAX
      //  ... nothing clobbered EAX.
      //  %ECX<def> = COPY %EAX
I.e., same def and src for both copies.

That being said, it seems strange to me that we have to support the full reg case that you exposed in the example. Indeed, this does not look like a copy propagation, but, like you said, a redundant copy. In other words, I would have expected we catch this in MachineCSE and if this is not the case, I believe it makes more sense to understand why and do it thereI.
Anyhow, MachineCSE would still be useful in cases where sub register are involved. I.e., I am fine with the code living here, but we would need a test case with sub register.
E.g.,
eax = ecx
al = cl


Repository:
  rL LLVM

http://reviews.llvm.org/D17475