[PATCH] D17474: MachineCopyPropagation: Keep scanning through instructions with regmasks

[Matthias Braun via llvm-commits]

MatzeB added inline comments.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:74
@@ -72,2 +73,3 @@
 
-void MachineCopyPropagation::SourceNoLongerAvailable(unsigned Reg) {
+void MachineCopyPropagation::ClobberCopySources(const RegList &Defs) {
+  for (unsigned Reg : Defs) {
----------------
junbuml wrote:
> I doubt if this method name well describe what it does. For me this method seems simply erase all (sub)regs in RegList from AvailCopyMap. 
Renamed it to removeRegsFromMap() and transformed it into a static function similar to removeClobberedRegsFromMap()

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:93
@@ -98,1 +92,3 @@
+    CopyMap.erase(*AI);
+    AvailCopyMap.erase(*AI);
   }
----------------
qcolombet wrote:
> Any reason why the two loops are not merged?
No, they previously just haven't been that close together and not 100% the same, so I didn't see it.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:129
@@ +128,3 @@
+    if (RegMask.clobbersPhysReg(Reg))
+      Map.erase(I);
+  }
----------------
qcolombet wrote:
> I was wondering if it would be more efficient to traverse the clobbered registers and erase the entries in the map accordingly, but I figured the map is usually pretty small.
> Maybe add a comment on that so that we don't have to think about it when we come across the code again.
Yes that it is indeed the reason, I will add a comment.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:286
@@ -293,1 +285,3 @@
+          ClobberCopySources(I->second);
+        SrcMap.erase(I);
       }
----------------
junbuml wrote:
> Maybe ?
> 
>   if (RegMask->clobbersPhysReg(I->first)) {
>     ClobberCopySources(I->second);
>     SrcMap.erase(I);
>   }
indeed, fixed.

================
Comment at: test/CodeGen/X86/machine-copy-prop.mir:1
@@ -1,2 +1,2 @@
-# RUN: llc -march=x86 -run-pass machine-cp -verify-machineinstrs -o /dev/null %s 2>&1 | FileCheck %s
+# RUN: llc -march=x86 -run-pass machine-cp -o /dev/null %s 2>&1 | FileCheck %s
 
----------------
qcolombet wrote:
> -verify-machineinstrs?
Turned out -verify-machineinstrs is a separate pass and won't run anyway when "llc -run-pass XXX" is used. Fixing that is for another commit.


Repository:
  rL LLVM

http://reviews.llvm.org/D17474