[PATCH] D30113: [AArch64] Extend AArch64RedundantCopyElimination to do simple copy propagation.

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

LGTM with nitpicks. Please wait for qcolombet though.



================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:84
 
+/// trackRegDefs - Remember what registers the specified instruction modifies.
+static void trackRegDefs(const MachineInstr &MI, BitVector &ClobberedRegs,
----------------
Don't repeat the function name in the doxygen comments.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:88-89
+  for (const MachineOperand &MO : MI.operands()) {
+    if (MO.isRegMask())
+      ClobberedRegs.setBitsNotInMask(MO.getRegMask());
+
----------------
could add a `continue;` here


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:118
+  // Registers that are known to contain zeros at the start of MBB.
+  SmallVector<unsigned, 4> TargetRegs;
+  // Registers clobbered in PredMBB between CompBr instruction and current
----------------
Use `MCPhysReg` instead of `unsigned`, possibly at a few more places below as well.

Why is this called TargetRegs? How about calling this `ZeroRegs` or `NullRegs` if you don't want it confused with xzr/wzr?


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:125
     --CompBr;
-    if (guaranteesZeroRegInBlock(*CompBr, MBB))
+    if (guaranteesZeroRegInBlock(*CompBr, MBB)) {
+      TargetRegs.push_back(CompBr->getOperand(0).getReg());
----------------
could this be
```
if (!guaranteesZeroRegInBlock(...))
  continue;
```
to reduce indentation?


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:169-171
+  // We've not found a known zero register, time to bail out.
+  if (TargetRegs.empty())
     return false;
----------------
Should this have an additional check for situations in which all target regs are clobbered (similar to the one a few lines earlier?)


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:190-191
+          if (TargetReg == DefReg || TRI->isSuperRegister(DefReg, TargetReg)) {
+            DEBUG(dbgs() << "Remove redundant Copy : ");
+            DEBUG((MI)->print(dbgs()));
+
----------------
Drive-by-comment (even though the patch doesn't touch it), this can be done simpler as:
`dbgs() << "Remove redundant Copy : " << *MI;`


================
Comment at: test/CodeGen/AArch64/machine-copy-remove.mir:3-12
+--- |
+  define void @test1() { ret void }
+  define void @test2() { ret void }
+  define void @test3() { ret void }
+  define void @test4() { ret void }
+  define void @test5() { ret void }
+  define void @test6() { ret void }
----------------
I recently found out that you can even leave out the whole IR string literal and MIRParser will create dummy functions for you, that should work here.

(Unfortunately as soon as you need to define other IR elements like globals or declaring external functions you need the IR block and MIRParser won't create dummy function for you right now) 


https://reviews.llvm.org/D30113