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

[David Li via llvm-commits]

davidxl added a comment.

Can you proceed with what Manman suggested (split it into smaller patches)? Here is my suggestion:

Patch-1: new interfaces -- no functional changes
Patch-2.1:  SelectionDAG builder change to use new interfaces
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;
----------------
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.

================
Comment at: include/llvm/Support/BranchProbability.h:41
@@ -40,3 +40,3 @@
   BranchProbability() : N(0) {}
   BranchProbability(uint32_t Numerator, uint32_t Denominator);
 
----------------
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?

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

================
Comment at: lib/CodeGen/BranchFolding.cpp:1110
@@ -1108,1 +1109,3 @@
 
+  // Scale down SumEdgeFreq to fit in a 32-bit integer.
+  int Scale = 0;
----------------
congh wrote:
> davidxl wrote:
> > Why don't we make the BranchProbability interface to take uint64_t input parameters? With that, there is no need to compute the scale here.
> This is explained above.
What if only 64bit interface is used?


http://reviews.llvm.org/D13745