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

[Xinliang David Li via llvm-commits]

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.



>
> > 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/efde2f9d/attachment.html>