[PATCH] D29344: [AArch64] Extend redundant copy elimination pass to handle non-zero stores.

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:121
+  // Must be an equality check (i.e., == or !=).
+  AArch64CC::CondCode CC = (AArch64CC::CondCode)(int)MI.getOperand(0).getImm();
+  if (CC != AArch64CC::EQ && CC != AArch64CC::NE)
----------------
Is it always safe to do this directly without any check for isConditionalBranch(), Opcode, or MI.getOperand(0).isImm()? 


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:137
+  // Finds compare instruction that sets NZCV used by conditional branch.
+  for (MachineBasicBlock::iterator B = PredMBB->begin(), I = MI; I != B;) {
+    --I;
----------------
If we didn't see any instruction that sets NZCV  between MI and  PredMBB->begin() and NZCV is live-in from PredMBB's single predecessor, then we could continue scan the single predecessor of PredMBB. Maybe different patch if it make sense.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:160
+      // The compare must not modify the register we're comparing against.
+      if (I->getOperand(0).getReg() == I->getOperand(1).getReg())
+        return false;
----------------
At this point we know that I->getOperand(0) is zero register, so this if statement will be taken only when I->getOperand(1) is zero register. I don't see any problem when both   getOperand(0) and getOperand(1) are zero register.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:222
 
   // We've not found a CBZ/CBNZ, time to bail out.
+  int64_t KnownImm;
----------------
Please update the comment as we continue checking non-zero case.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:233
     return false;
+
   assert(TargetRegisterInfo::isPhysicalRegister(TargetReg) &&
----------------
Is there any case where both ZeroRegVal and KnownRegVal is true? If not, adding assert() will be good.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:264
       unsigned SrcReg = MI->getOperand(1).getReg();
-
-      if ((SrcReg == AArch64::XZR || SrcReg == AArch64::WZR) &&
-          !MRI->isReserved(DefReg) &&
+      if (ZeroRegVal && !MRI->isReserved(DefReg) &&
+          (SrcReg == AArch64::XZR || SrcReg == AArch64::WZR) &&
----------------
We could  move check for ZeroRegVal above if statement in line 259. 


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:283
+    //     ret
+    if (MI->isMoveImmediate() && MI->getOperand(0).isReg() &&
+        MI->getOperand(1).isImm()) {
----------------
The comment in MachineInstr.h mention that isMoveImmediate() return true for a move immediate (including conditional moves).  I think the conditional move is not the case for AArch64, but just want to double confirm. 


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:283
+    //     ret
+    if (MI->isMoveImmediate() && MI->getOperand(0).isReg() &&
+        MI->getOperand(1).isImm()) {
----------------
junbuml wrote:
> The comment in MachineInstr.h mention that isMoveImmediate() return true for a move immediate (including conditional moves).  I think the conditional move is not the case for AArch64, but just want to double confirm. 
We can move KnownRegVal in line 283.  It seems that both ZeroRegVal and KnownRegVal cannot be true at the same time.  Then we can use if-else, instead of if-if.


https://reviews.llvm.org/D29344