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

[Matthias Braun via llvm-commits]

MatzeB added inline comments.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:146
@@ +145,3 @@
+    MachineInstr *CopyMI = CI->second;
+    if (!MRI->isReserved(Def) && isNopCopy(CopyMI, Def, Src, TRI) &&
+        (!MRI->isReserved(Src) ||
----------------
junbuml wrote:
> In MRI->isReserved(Def), we should use the actual register defined in MI, instead of the Def passed as a parameter. In case of the second call for eraseRedundantCopy() below in line 209, MI's Src is passed as Def here. 
>  
I think we cannot eliminate any copy to/from a reserved reg, that's what I put in place now.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:155
@@ +154,3 @@
+           : make_range(CopyMI->getIterator(), MI->getIterator()))
+        MMI.clearRegisterKills(Def, TRI);
+
----------------
junbuml wrote:
> Cleaning kill for Def is not always correct. In the second call for eraseRedundantCopy() below,  MI's Src is passed as the second parameter, which is Def here, but we need to clean kill for MI's Def instead of  MI's Src. 
Good catch! Should be fixed now.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:209
@@ +208,3 @@
+      // %RAX<def> = COPY %RSP
+      if (eraseRedundantCopy(MI, Def, Src))
+        continue;
----------------
qcolombet wrote:
> 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
I reorganized the whole comment hope it is easier to understand now.


Repository:
  rL LLVM

http://reviews.llvm.org/D17475