[PATCH] D16203: [AArch64] Remove copy zero after cbz/cbnz

[Chad Rosier via llvm-commits]

mcrosier added inline comments.

================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:342
@@ -341,3 +341,3 @@
 
-  for (MachineFunction::iterator I = MF.begin(), E = MF.end(); I != E; ++I)
+  for (MachineFunction::iterator I = MF.begin(), E = MF.end(); I != E; ++I) {
     Changed |= CopyPropagateBlock(*I);
----------------
Range-based loop?

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:985
@@ -984,1 +984,3 @@
 
+// This function try to remove copy instructions placed in a target block of
+// the cbz/cbnz and they copy zero to the same register used in cbz/cbnz.
----------------
What about something like this:
// This function removes unnecessary zero copies from BBs that are targets of
// cbz/cbnz instructions. For instance, the copy instruction in below code can
// be removed because the cbz jumps to BB#2 when w0 is zero.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1031
@@ +1030,3 @@
+  // Remove redundant Copy instructions unless TargetReg is modified.
+  for (MachineBasicBlock::iterator I = MBB->begin(), E = MBB->end(); I != E;) {
+    MachineInstr *MI = &*I;
----------------
Range-based loop?

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.h:167
@@ +166,3 @@
+
+  // optimizeCopy - Remove redundant copy instructions which remain after
+  // register allocation.
----------------
Please use doxygen (i.e., ///) style comments.  Also, you should remove the function name from the comment as Doxygen knows what to do (see r253029 as an example).

================
Comment at: test/CodeGen/AArch64/machine-copy-remove.ll:57
@@ +56,3 @@
+
+; CHECK-LABEL: f_WX
+; CHECK: cbz w[[REG:[0-9]+]], [[BB:.LBB.*]]
----------------
I'd suggest adding a comment to this case mention that the transform isn't safe because we don't know if the upper bits of the X register are zero.


http://reviews.llvm.org/D16203