[PATCH] ARM64 - Conditional branch peephole

[Renato Golin]

Hi Gerolf,

Overall, the patch looks good. I think the substitution is sound, but I'd rather Tim had a look to confirm. 

There are a few in-line comments that need addressing...

cheers,
--renato

================
Comment at: lib/CodeGen/PeepholeOptimizer.cpp:506
@@ +505,3 @@
+bool PeepholeOptimizer::optimizeCondBranch(MachineInstr *MI) {
+  if (!TII->optimizeCondBranch(MI))
+    return false;
----------------
Couldn't you just...

    return TII->optimizeCondBranch(MI);

?

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2867
@@ +2866,3 @@
+  bool has_bit_test = false;
+  // target BB operand in MI
+  unsigned tbb_operand = 0;
----------------
Same here with comments. If you call your variables more explicitly, you don't need the one-line comment before.

Ideas are:

    bool IsNegativeBranch;
    bool IsTestAndBranch;
    bool TargetsBBInMI;

Also, the coding standard mandates camel case for variable and function names, with caps on first letter for variables and low-caps for function names.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2871
@@ +2870,3 @@
+  unsigned cc_operand = 0;
+  switch (MI->getOpcode()) {
+  default:
----------------
Personal opinion, I don't like switches for this kind of thing. I think repeated ifs are more explicit to not repeat the assignments, at least it's more clear what each flag means from instructions. But I'm ok with the switch.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2880
@@ +2879,3 @@
+    cc_operand = 2;
+
+    break;
----------------
unnecessary space

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2902
@@ +2901,3 @@
+  }
+  assert(cc_operand == 2 || cc_operand == 3 && "Invalid CC");
+  assert(tbb_operand == 1 || cc_operand == 2 && "Invalid TBB");
----------------
I don't think it's possible for this to happen, since they're local variables and you're setting them above with an llvm_unreachable. These asserts are completely redundant.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2913
@@ +2912,3 @@
+  MachineBasicBlock *MBB = MI->getParent();
+  assert(MBB && "Can't get MachineBasicBlock here");
+  MachineFunction *MF = MBB->getParent();
----------------
while I generally support asserts, I think you got too many of them here... It's making the rest of the code unreadable

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2950
@@ +2949,3 @@
+    BuildMI(MBB, MI, DL, get(AArch64::Bcc)).addImm(CC).addMBB(TBB);
+    DefMI->eraseFromParent();
+    MI->eraseFromParent();
----------------
Did DefMI get inserted anywhere? I may be wrong, but I don't think this is necessary

http://reviews.llvm.org/D5637