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

[Jun Bum Lim via llvm-commits]

junbuml added inline comments.

================
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.
----------------
mcrosier wrote:
> 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.
Thanks! Looks better !

================
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;
----------------
mcrosier wrote:
> Range-based loop?
I don't think it's possible because we remove MI in the loop? Please correct me if I'm wrong.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.h:167
@@ +166,3 @@
+
+  // optimizeCopy - Remove redundant copy instructions which remain after
+  // register allocation.
----------------
mcrosier wrote:
> 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).
Oh.. Thanks. I will fix it.

================
Comment at: test/CodeGen/AArch64/machine-copy-remove.ll:57
@@ +56,3 @@
+
+; CHECK-LABEL: f_WX
+; CHECK: cbz w[[REG:[0-9]+]], [[BB:.LBB.*]]
----------------
mcrosier wrote:
> 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.
I will do it ! 


http://reviews.llvm.org/D16203