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

[Geoff Berry via Phabricator via llvm-commits]

gberry added a comment.

A few minor comments below.  Two questions:

- can we avoid doing two backward scans of the pred block?  perhaps just tracking the clobbers once
- can you add some negative tests that check that clobbers inhibit this optimization when expected?



================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:114
+/// Check if the basic block \p MBB is the target block to which a conditional
+/// branch \p CondBr jumps and whos equality comparison is against a constant.
+/// If true, return the compare instruction the branch is predicated upon.
----------------
Typo 'whos' -> 'whose'


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:57
+
+  typedef struct RegImm {
+    MCPhysReg Reg;
----------------
This typedef seems unnecessary.  Why not just struct RegImm?


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:59
+    MCPhysReg Reg;
+    int64_t Imm;
+    RegImm(MCPhysReg Reg, int64_t Imm) : Reg(Reg), Imm(Imm) {}
----------------
This could be int32_t to save some space since SUBS can only reference 12-bit immediates (24 with lsl 12 which isn't currently handled by this change), but it probably isn't worth it since there aren't many RegImm objects.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:171
+      // The src register must not be modified between the cmp and conditional
+      // branch.  This include a self-clobbering compare.
+      if (KnownRegValClobberedRegs[PredI.getOperand(1).getReg()] ||
----------------
Typo 'include' -> 'includes'


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:226
+      FirstUse = Itr;
+    } else
       continue;
----------------
Perhaps invert the condition above and put the continue inside it to get rid of the 'else' here


https://reviews.llvm.org/D29344