[PATCH] D12603: Use fixed-point representation for BranchProbability

[Xinliang David Li via llvm-commits]

You explanation makes sense -- I saw the other patch to fix the
rounding bug in ifcvt.

David

On Wed, Sep 9, 2015 at 3:08 PM, Cong Hou <congh at google.com> wrote:
> congh added inline comments.
>
> ================
> Comment at: test/CodeGen/AArch64/aarch64-deferred-spilling.ll:26
> @@ -27,1 +25,3 @@
> +; REGULAR-NOT: {{[wx]}}21{{,?}}
> +; REGUAL: ldr w21, [sp, #[[OFFSET]]] // 4-byte Folded Reload
>  ;
> ----------------
> davidxl wrote:
>> register name change?
> Fixed. This test case keeps unchanged after the patch update. See explains below.
>
> ================
> Comment at: test/CodeGen/ARM/cmpxchg-weak.ll:11
> @@ -10,3 +10,3 @@
>  ; CHECK:     cmp     [[LOADED]], r1
> -; CHECK:     strexeq [[SUCCESS:r[0-9]+]], r2, [r0]
> -; CHECK:     cmpeq   [[SUCCESS]], #0
> +; CHECK:     strex   [[SUCCESS:r[0-9]+]], r2, [r0]
> +; CHECK:     cmp     [[SUCCESS]], #0
> ----------------
> davidxl wrote:
>> unrelated change?
> This change is caused by fixed point probability representation. After investigating the root cause, I found that it is from if-conversion. More specifically, when determine whether to do if-conversion there is a cost model measurement in which the branch probability is used to scale some cost (in integers). The current scale method does not round the result, which may be a problem. For example, ARMBaseInstrInfo::isProfitableToIfCvt() has the following statements:
>
>
> ```
>   unsigned TUnpredCost = Probability.scale(TCycles);
>   unsigned FUnpredCost = Probability.getCompl().scale(FCycles);
>   unsigned UnpredCost = TUnpredCost + FUnpredCost;
> ```
>
> Now assume Probability is 0.5 and both TCycles and FCycles are 1. If scale() does not round, then TUnpredCost and FUnpredCost are 0, and hence UnpredCost = TUnpredCost + FUnpredCost is also zero. This doesn't make sense as here if we calculate the cost in floating point we should get ~1 as UnpredCost.
>
> We we do round in scale(), it is possible to get both TUnpredCost and FUnpredCost as 1, which is also incorrect. However, in our fixed point representation, 0.5 is actually a little less than 0.5, and its complement is a little greater than 0.5. Therefore we will have TUnpredCost = 0, and FUnpredCost = 1 after rounding. This is very hacky.
>
> So I think we do need scale() to have rounding behavior, but in ARMBaseInstrInfo::isProfitableToIfCvt() to avoid precision related issues it is better to scale TCycles/FCycles up (e.g. x1000) before scaling them by Probability. This is find as the function returns the value of
>
>
> ```
> return (TCycles + FCycles + TExtra + FExtra) <= UnpredCost;
>
> ```
> and we can scale every operands up.
>
> What do you think?
>
>
> http://reviews.llvm.org/D12603
>
>
>