[PATCH] D13745: Replace all uses of branch weights by branch probabilities on machine code level passes.

[Cong Hou via llvm-commits]

On Mon, Oct 19, 2015 at 1:55 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Mon, Oct 19, 2015 at 1:08 PM, Cong Hou <congh at google.com> wrote:
>
>> congh added a comment.
>>
>> In http://reviews.llvm.org/D13745#268627, @davidxl wrote:
>>
>> > Can you proceed with what Manman suggested (split it into smaller
>> patches)? Here is my suggestion:
>> >
>> > Patch-1: new interfaces -- no functional changes
>>
>>
>> This may increase the storage space of some classes like MBB which will
>> store both weights and probabilities. Is that OK?
>>
>
> How much memory overhead do you see? I expect it to be low.
>

This doubles the storage of weights in MBB but overall the overhead is not
too high considering other MBB members like pred/succ-list and live-ins.

Cong


>
>
>
>>
>> > Patch-2.1:  SelectionDAG builder change to use new interfaces
>>
>>
>> So for other users we just treat probabilities (which is built in
>> SelectionDAGBuilder by using the new interfaces) as weights, right?
>>
>
> yes -- something in that line.
>
>
>>
>> > Patch-2.2:  rest of the clients
>>
>> >  Patch-3: remove old weight based interfaces (no functional change
>> expected)
>>
>>
>>
>>
>>
>> ================
>> Comment at: include/llvm/Analysis/BranchProbabilityInfo.h:93
>> @@ -89,3 +92,3 @@
>>    /// are updating the analysis by later calling setEdgeWeight.
>>    uint32_t getEdgeWeight(const BasicBlock *Src,
>>                           unsigned IndexInSuccessors) const;
>> ----------------
>> davidxl wrote:
>> > congh wrote:
>> > > davidxl wrote:
>> > > > Why is this interface still kept? As in MBB, this one should also
>> be deprecated.
>> > > This is needed in BlockFrequencyInfoImpl. Note that in this patch I
>> only changed edge weights into probabilities in MBB but not in
>> BranchProbabilityInfo. Do you think if we should also do this in
>> BranchProbabilityInfo?
>> > yes, this should also be done for BPI -- but in another patch after
>> this one gets in and stablizes.
>> OK.
>>
>> ================
>> Comment at: include/llvm/Support/BranchProbability.h:41
>> @@ -40,3 +40,3 @@
>>    BranchProbability() : N(0) {}
>>    BranchProbability(uint32_t Numerator, uint32_t Denominator);
>>
>> ----------------
>> davidxl wrote:
>> > congh wrote:
>> > > davidxl wrote:
>> > > > Any reason why Numerator and Denominator can not be uint64_t?
>> > > If we add an overload constructor with type uint64_t, there will be
>> many compilation errors of ambiguous overloads when we pass arguments of
>> int types.
>> > How about removing the interface with 32bit parameter and just keep the
>> 64bit one?
>> At this momenta it seems that we only need 64-bit as input in one place.
>> So is it worthwhile to do this? This may bring overhead when constructing
>> probabilities.
>
>
> Actually current 32bit based interface does unnecessary casting that can
> be eliminated, so I think it is worth while. A side note, I think that
> constructor needs to be an inline function defined in the header.
>
>
>
>> Another solution is creating a static builder function that takes 64-bit
>> as inputs.
>>
>
> Probably not worth it.
>
>
> David
>
>
>>
>> ================
>> Comment at: include/llvm/Support/BranchProbability.h:83
>> @@ -82,4 +82,3 @@
>>    BranchProbability &operator+=(BranchProbability RHS) {
>> -    assert(N <= D - RHS.N &&
>> -           "The sum of branch probabilities should not exceed one!");
>> -    N += RHS.N;
>> +    // Saturate the result in case of overflow.
>> +    N = (N + RHS.N < N) ? D : N + RHS.N;
>> ----------------
>> davidxl wrote:
>> > congh wrote:
>> > > davidxl wrote:
>> > > > when can overflow happen? Also should the check really be:
>> > > >
>> > > > (uint64_t)N + RHS.H > D ?
>> > > When we construct a branch probability we do rounding to get the
>> numerator. If the denominator is odd, then 1/2 + 1/2 will exceed 1 using
>> BranchProbability representation. That is when overflow happens.
>> > >
>> > > I forgot to update this part as when it is written we were using
>> UINT32_MAX as denominator. I will update it.
>> > ok -- but the check does not handle the case when the resulting Prob >
>> 1.
>> We don't allow a probability to be greater than 1, so here we use the
>> saturating operation:
>>
>> N = ((uint64_t)N + RHS.N > D) ? D : N + RHS.N;
>>
>>
>> http://reviews.llvm.org/D13745
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151019/d5225799/attachment.html>