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

[Chad Rosier via Phabricator via llvm-commits]

mcrosier marked 12 inline comments as done.
mcrosier added inline comments.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:120
+  // 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)
----------------
MatzeB wrote:
> Is that cast to `(int)` here necessary?
It is not.  Will remove.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:146-148
+      unsigned DestReg = I->getOperand(0).getReg();
+      if (DestReg != AArch64::WZR && DestReg != AArch64::XZR)
+        return false;
----------------
MatzeB wrote:
> does the DestReg matter here? I would assume this transformation to work as well when we write to an actual register...
No, this is a simplification not a requirement.  I'll remove this check.  I think the only necessary check is to make sure the compare doesn't clobber itself (e.g., SUBS x1, x1, 5).


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:155
+      // For simplicity, give up on non-zero shifts.
+      if (I->getOperand(3).getImm())
+        return false;
----------------
MatzeB wrote:
> Somewhat off-topic from this specific patch and I don't expect this to be fixed here: It feels bad to scatter all those magic values like the `3` here around the targets, this function is another example where we have a lot of magic input numbers. At some point we should add some getter/accessor functions somewhere to give them a name or extend tablegen to generate some enum constants for us...
I agree.


https://reviews.llvm.org/D29344