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

[Chad Rosier via Phabricator via llvm-commits]

mcrosier 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)
----------------
junbuml wrote:
> Is it always safe to do this directly without any check for isConditionalBranch(), Opcode, or MI.getOperand(0).isImm()? 
Yes, I believe so.  We directly check that the opcode is Bcc above, so we know MI must be a conditional branch at this point.  AFAIK, the 0 operand is always the CC for a Bcc.


================
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;
----------------
junbuml wrote:
> 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.
True, but I'd prefer to defer this for another day as you suggest.


================
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;
----------------
junbuml wrote:
> 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.
You're correct.  I'll remove accordingly.


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


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


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:283
+    //     ret
+    if (MI->isMoveImmediate() && MI->getOperand(0).isReg() &&
+        MI->getOperand(1).isImm()) {
----------------
junbuml wrote:
> 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.
For AArch64, only the MOVi32imm and MOVi64imm pseudos will return true for isMoveImmediate() and these instructions are unconditionally executed.


https://reviews.llvm.org/D29344