[PATCH] D18572: [AArch64] Relax branches by fusing compare with conditional branch when we can infer that source register is zero/non-zero.

[Tim Northover via llvm-commits]

t.p.northover added inline comments.

================
Comment at: lib/Target/AArch64/AArch64BranchRelaxation.cpp:483
@@ +482,3 @@
+  if (!Compare->getOperand(1).isReg() || !Compare->getOperand(2).isImm() ||
+      Compare->getOperand(2).getImm() != 1)
+    return false;
----------------
What about "cmp reg, #0"/"b.le ..."?

================
Comment at: lib/Target/AArch64/AArch64BranchRelaxation.cpp:491
@@ +490,3 @@
+  MachineBasicBlock &MBB = *Compare->getParent();
+  unsigned SrcReg = Compare->getOperand(0).getReg();
+  unsigned SrcReg2 = Compare->getOperand(1).getReg();
----------------
Operand 0 is a destination, isn't it? Usually XZR/WZR in a true compare. Actually, I think we want to make sure it's <dead> if we're going to be removing this definition.

================
Comment at: lib/Target/AArch64/AArch64BranchRelaxation.cpp:510
@@ +509,3 @@
+          PI.getOperand(0).isUse() && PI.getOperand(0).getReg() == SrcReg2)
+        LastUse = &PI;
+    }
----------------
This seems wrong on 2 levels. First, if there are multiple predecessors then the fact that one of them ends in a TBZ says nothing about the contents of SrcReg2 coming from any others.

But even if that wasn't the case we really wouldn't want the logic to rely on the order MBB's predecessors happen to be returned in.

================
Comment at: lib/Target/AArch64/AArch64BranchRelaxation.cpp:568
@@ +567,3 @@
+
+    bool CompleteNZCVUsers = !isNZCVLiveOut(MBB);
+    SmallVector<MachineInstr *, 4> NZCVUsers;
----------------
Related to two other comments about this loop: this seems like an immediate continue condition. In general, it looks likely that once you prune out the extra logic `CompleteNZCVUsers` will be unnecessary.

================
Comment at: lib/Target/AArch64/AArch64BranchRelaxation.cpp:580
@@ +579,3 @@
+        NZCVUsers.clear();
+        continue;
+      }
----------------
break? It doesn't seem like you could ever really do more in this BB.

================
Comment at: lib/Target/AArch64/AArch64BranchRelaxation.cpp:583-585
@@ +582,5 @@
+
+      if (MI->definesRegister(AArch64::NZCV)) {
+        NZCVUsers.clear();
+        CompleteNZCVUsers = true;
+      }
----------------
Isn't this also an immediate break condition? We've hit something that defines NZCV but isn't a compare. Even if there's a "cmp reg, #1" above we mustn't optimize it.


http://reviews.llvm.org/D18572