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

[Cong Hou via llvm-commits]

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?

> 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?

> 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. Another solution is creating a static builder function that takes 64-bit as inputs.

================
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