[PATCH] D16203: [AArch64] Remove copy zero after cbz/cbnz

[Matthias Braun via llvm-commits]

MatzeB requested changes to this revision.
MatzeB added a comment.
This revision now requires changes to proceed.

I don't like the integration into MachineCopyPropagation here, the algorithm is not using any other information or methods of that pass (except for the fact that the pass iterates over all basic blocks). This is not enough reason to introduce a new TII callback. You can just as well make this a standalone aarch64 specific pass.

The scope of the algorithm could be extended, I don't see why it should not work for cases like "cmp reg, #CC; b.(n)e XX ... mov reg, #CC" (i.e. any reg getting compared equal/unequal to a constant). In principle this could also be extended to check the whole dominance subtree below the comparison though I guess it is not worth the compiletime to compute a dominance tree here just for that.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:342
@@ -341,4 +341,3 @@
 
-  for (MachineFunction::iterator I = MF.begin(), E = MF.end(); I != E; ++I)
-    Changed |= CopyPropagateBlock(*I);
-
+  for (auto &I : MF) {
+    Changed |= CopyPropagateBlock(I);
----------------
Using "MachineInstr &MI" instead of "auto &I" would be friendlier to the reader.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:995
@@ +994,3 @@
+                                    const TargetRegisterInfo *TRI) const {
+  bool Changed = false;
+  // Check if the current basic block has a single predecessor.
----------------
I would move this downwards closer to the point where it is first assigned.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1052-1054
@@ +1051,5 @@
+
+    // Stop if encountering any intervening side effect.
+    if (MI->hasUnmodeledSideEffects() || MI->isCall() || MI->isTerminator())
+      return Changed;
+
----------------
These things cannot simply change the value of registers if they do they need additional MachineOperands with defs, uses, regmasks which already be handled below.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1056-1057
@@ +1055,4 @@
+
+    for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
+      MachineOperand &MO = MI->getOperand(i);
+      // FIXME: It is possible to use the register mask to check if all
----------------
This could be `for (const MachineOperand &MO : MI->operands())`

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1066-1067
@@ +1065,4 @@
+      unsigned Reg = MO.getReg();
+      if (!Reg)
+        continue;
+      assert(TargetRegisterInfo::isPhysicalRegister(Reg) &&
----------------
This check can be moved into the assert(), as TargetRegs.count(0) will always be false.


http://reviews.llvm.org/D16203