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

[Cong Hou via llvm-commits]

congh added inline comments.

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

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:426
@@ -424,2 +425,3 @@
   ///
   /// Note that duplicate Machine CFG edges are not allowed.
+  void addSuccessor(MachineBasicBlock *Succ, BranchProbability Prob);
----------------
davidxl wrote:
> A side note -- I remember you mention in if conversion, duplicate cfg edges do exist?
I am not quite sure about it.

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:433
@@ +432,3 @@
+  /// to keep their relative relations.
+  void addSuccessor(MachineBasicBlock *Succ);
+
----------------
manmanren wrote:
> davidxl wrote:
> > Why do we want to have this interface?  If the client does not know what probability to use, it won't be too hard to specify 1/N either.
> We seem to normalize the probabilities in this version of addSuccessor, but not the previous one. Any reason?
This interface will do probability normalization which makes sure that the sum of them is around 1. Then other interface won't do this.

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

================
Comment at: include/llvm/Support/BranchProbability.h:45
@@ -44,3 +44,3 @@
 
   static BranchProbability getZero() { return BranchProbability(0); }
   static BranchProbability getOne() { return BranchProbability(D); }
----------------
davidxl wrote:
> It is useful to introduce a static helper method (templatized) to return a default BranchProbability 
> 
> template<class BlockT> BranchProbability getDefaultProbabillity(BlockT *);
I doubt if this is useful. Before we add a new successor, the number of successors n is an old number, and the default probability we need is actually 1/(n+1). Returning 1/(n+1) from this function is confusing.

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

================
Comment at: lib/CodeGen/BranchFolding.cpp:1110
@@ -1108,1 +1109,3 @@
 
+  // Scale down SumEdgeFreq to fit in a 32-bit integer.
+  int Scale = 0;
----------------
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.

================
Comment at: lib/CodeGen/MIRPrinter.cpp:465
@@ -466,1 +464,3 @@
+      if (MBB.hasSuccessorProbabilities())
+        OS << '(' << MBB.getSuccProbability(I) << ')';
     }
----------------
davidxl wrote:
> Avoid using the private method getSuccProbability here?
I will try to replace it with another method in another patch.

================
Comment at: lib/CodeGen/MachineBranchProbabilityInfo.cpp:44
@@ +43,3 @@
+  auto Prob = Src->getSuccProbability(Dst);
+  if (Prob.isZero())
+    return BranchProbability(1, Src->succ_size());
----------------
davidxl wrote:
> Is this correct? There are cases where the probability is really zero -- you need a better way to tell the difference between this and Probs.empty() case.
> 
> How about changing getSuccProbabliy's interface to be
> 
> bool getSuccProbability(BranchProbability&);
> 
> it returns false when there is no information.
This interface looks good. Let me see if I could return a default probability if there is no information from getSuccProbability(). Or can we represent zero probability by using 1 as numerator?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1498
@@ +1497,3 @@
+    // the number of successors.
+    auto SuccSize = std::max<uint32_t>(
+        std::distance(succ_begin(SrcBB), succ_end(SrcBB)), 1);
----------------
davidxl wrote:
> Use a helper method for this (getDefaultProbability)
It seems that there is no other places that need this helper. If I found other places I will create a helper.


http://reviews.llvm.org/D13745