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

[Geoff Berry via Phabricator via llvm-commits]

gberry added a comment.

I'm convinced the two passes over PredBB makes sense.

Could you add a couple of test cases where the known register and the eliminated MOV are different register sizes?  I feel like we may get into trouble in the case where we're depending on the MOVi32imm zeroing out the high 32-bits of a 64-bit value.  For example:
b1:

  CMP w0, 1
  BCC EQ, b2

b2:

  w0 = MOVi32imm 1
  STORE x0 # depends on high 32-bits of w0 being zero'd by MOVi32imm

A few more questions/suggestions below.



================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:62
+
+  bool knownRegValInBlock(MachineInstr &CondBr, MachineBasicBlock *MBB,
+                          MCPhysReg &KnownReg, int32_t &ImmVal);
----------------
Perhaps this could return an Optional<RegImm>, then you could get rid of the two out ref parameters?


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:173
+          (DestReg != AArch64::WZR && DestReg != AArch64::XZR &&
+           DestReg == SrcReg))
+        return false;
----------------
I think you could get rid of lines 172 and 173 if you do the trackRegDefs before the ClobberedRegs check.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:215
   // instruction being checked in loop.
   ClobberedRegs.reset();
+
----------------
I believe this is no longer needed here.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:355
   // but should be okay since kill markers are being phased out.
   for (MachineInstr &MMI : make_range(FirstUse, PredMBB->end()))
     MMI.clearKillInfo();
----------------
I don't think FirstUse is being calculated correctly in the case of the non-zero immediate.  E.g. if there are no COPYs to worry about, FirstUse should be the SUBS compare instruction since we want to remove the kill of the known value register in that case if it is present.  Your test case test10 should show this problem, though I'm not sure why -verfiy-machineinstrs isn't catching it.



https://reviews.llvm.org/D29344