[PATCH] D34952: [ARM] Adjust ifcvt heuristic for the diamond ifcvt case

[John Brawn via Phabricator via llvm-commits]

john.brawn added inline comments.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:1885
+      // discount it from PredCost.
+      PredCost -= 1 * ScalingUpFactor;
     }
----------------
samparker wrote:
> Does the '1' represent the single branch instruction that we will not have to execute? Does it not have to be the number of cycles saved?
IfConversion sets the cost of an instruction to be 1 + (latency-1) if latency > 1, but looking at the various sched models latency of branches is either 0 or 1 as a branch doesn't produce any result. Also getting the latency of an instruction requires having a TargetSchedModel which isn't available here, so I'd have to plumb it in from IfConversion.


================
Comment at: test/CodeGen/Thumb2/ifcvt-no-branch-predictor.ll:98
 ; CHECK-LABEL: diamond2:
-; CHECK-BP: itte
-; CHECK-BP: streq
-; CHECK-BP: ldreq
-; CHECK-BP: strne
-; CHECK-NOBP: cbz
-; CHECK-NOBP: str
-; CHECK-NOBP: b
-; CHECK-NOBP: str
-; CHECK-NOBP: ldr
+; CHECK-BP: cbz
+; CHECK-BP: str
----------------
samparker wrote:
> why does this patch affect targets with a branch predictor?
It doesn't, I've adjusted the function (see the extra store in if.then below) to make sure we see a difference between bp and nobp (because with the new heuristic both produced the same code for the current version of this function).


Repository:
  rL LLVM

https://reviews.llvm.org/D34952