[PATCH] ARM64 - Conditional branch peephole

[Gerolf Hoflehner]

Renato and Tim thanks for your feedback. I followed up on all but the switch vs if comment.

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

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2867
@@ +2866,3 @@
+  bool has_bit_test = false;
+  // target BB operand in MI
+  unsigned tbb_operand = 0;
----------------
rengolin wrote:
> 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.
yes, thanks.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2871
@@ +2870,3 @@
+  unsigned cc_operand = 0;
+  switch (MI->getOpcode()) {
+  default:
----------------
rengolin wrote:
> 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.
Thanks. I'm not in favor of such code either, but don't see significant improvement with the ifs. Laziness wins! :-)

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

================
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");
----------------
rengolin wrote:
> 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.
removed.

================
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();
----------------
rengolin wrote:
> while I generally support asserts, I think you got too many of them here... It's making the rest of the code unreadable
agree. Removed all but one.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2942-2943
@@ +2941,4 @@
+  // instructions in between.
+  if (!modifiesConditionCode(DefMI, MI, CheckOnlyCCWrites,
+                             &getRegisterInfo())) {
+    MachineBasicBlock &MBB = *MI->getParent();
----------------
t.p.northover wrote:
> We usually prefer to bail early in failure cases:
> 
>     if (modifiesConditionCode(...))
>       return false;
>     [...]
The only reason I did this way was was to make Build happy that need a reference to MBB. But I can introduce a new variable for it and follow the preference.

================
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();
----------------
t.p.northover wrote:
> rengolin wrote:
> > Did DefMI get inserted anywhere? I may be wrong, but I don't think this is necessary
> It's already going to be in the basic block, isn't it? It's not one we created.
Yes, that  line should go or be fixed: It should be dead code when MI is removed. If it is not dead code it needs an additional check that MI is the only use.

http://reviews.llvm.org/D5637