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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

This looks good to me when the comments below are addressed.



================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:84-86
+/// Remember what registers the specified instruction modifies.
+static void trackRegDefs(const MachineInstr &MI, BitVector &ModifiedRegs,
+                         const TargetRegisterInfo *TRI) {
----------------
Looks like you synced that with https://reviews.llvm.org/D30113, good.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:105-108
+// Check if the current basic block is the target block to which a conditional
+// branch instruction jumps and whos equality comparison is against a constant.
+// If not, return false.  Otherwise, return true after setting the TargetReg
+// and known ImmVal.
----------------
Use doxygen `///` comment. It would help to talk about `block \p MBB` instead of `current block`, similarily to explain some of the other parameters.


================
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)
----------------
Is that cast to `(int)` here necessary?


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:129-130
+
+  if (MI == PredMBB->begin())
+    return false;
+
----------------
Add a comment that this is a shortcut for a block that is empty except for the branch.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:136
+  // Finds compare instruction that sets NZCV used by conditional branch.
+  for (MachineBasicBlock::iterator B = PredMBB->begin(), I = MI; I != B;) {
+    --I;
----------------
This slightly confused me as most loops in LLVM first initialize/declare the iteration variable then the end variable.

This could also be written as a range based for: `for (MachineInstr &PredMI : make_range(std::next(MI.getReverseIterator()), PredMBB->rend())`.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:146-148
+      unsigned DestReg = I->getOperand(0).getReg();
+      if (DestReg != AArch64::WZR && DestReg != AArch64::XZR)
+        return false;
----------------
does the DestReg matter here? I would assume this transformation to work as well when we write to an actual register...


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:155
+      // For simplicity, give up on non-zero shifts.
+      if (I->getOperand(3).getImm())
+        return false;
----------------
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...


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:170
+
+    // Bail if we see an instruction that defines NZCV, but we don't handle.
+    if (I->definesRegister(AArch64::NZCV))
----------------
typo but->that


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:182-183
+    MachineInstr *MI, unsigned DefReg, unsigned &SmallestDef) {
+  DEBUG(dbgs() << "Remove redundant copy : ");
+  DEBUG((MI)->print(dbgs()));
+
----------------
You can use `DEBUG(dbgs() << "Remove redundant copy: " << *MI);`


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:218
+  int64_t KnownImm;
+  bool KnownRegVal = false;
+  if (!ZeroRegVal) {
----------------
Can't you just have a single variable `KnownVal` or similar here? You can then use `KnownVal == 0` instead of ZeroRegVal.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:271-280
+    // Case 2:
+    //   BB#0:
+    //     cmp x0, #1
+    //     b.eq .LBB0_2
+    //   BB#1:
+    //     ldr x0, [x1]
+    //     ret
----------------
Do we really need to split this into Case 1/Case 2? There seems to be more equal code than different one so I would expect this to be better merged (with a slightly more complicated condition to check).


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:328
+  BitVector ModifiedRegs;
+  ModifiedRegs.resize(TRI->getNumRegs());
+
----------------
You may even delay the resizing/allocation of the bitvector to the point where it is actually used (resizing it multiple times to the same size shouldn't hurt performance).


================
Comment at: test/CodeGen/AArch64/machine-copy-remove.mir:1
+# RUN: llc -mtriple=aarch64--linux-gnu -run-pass=aarch64-copyelim %s -verify-machineinstrs -o - 2>&1 | FileCheck %s
+
----------------
The `2>&1` is no longer necessary for .mir tests and is discouraged now as it hides abort()/assert() messages if they happen.


================
Comment at: test/CodeGen/AArch64/machine-copy-remove.mir:5-8
+--- |
+  define void @test1() { ret void }
+  define void @test2() { ret void }
+...
----------------
The IR block can be left out completely in this case the MIRParser will create the dummy functions.


================
Comment at: test/CodeGen/AArch64/machine-copy-remove.mir:14
+  bb.0:
+    successors: %bb.1(0x40000000), %bb.2(0x40000000)
+    liveins: %w0, %x1
----------------
This can be slightly simplified by leaving out the block frequencies (i.e. just `successors: %bb.1, %bb.2` Similar for the other blocks.


================
Comment at: test/CodeGen/AArch64/machine-copy-remove.mir:38
+...
+# CHECK-LABEL: test2
+# CHECK: bb.1:
----------------
Typo!
I also tend to use `CHECK-LABEL: name: test1` to better match the mir output with the typo this probably matched the dummy IR function instead of the beginning of the actual test1 mir function.


https://reviews.llvm.org/D29344