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

[David Li via llvm-commits]

davidxl 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;
----------------
Why is this interface still kept? As in MBB, this one should also be deprecated.

================
Comment at: include/llvm/Analysis/BranchProbabilityInfo.h:100
@@ -96,3 +99,3 @@
   /// It is guaranteed to fall between 1 and UINT32_MAX.
   uint32_t getEdgeWeight(const BasicBlock *Src, const BasicBlock *Dst) const;
 
----------------
Same here

================
Comment at: include/llvm/Analysis/BranchProbabilityInfo.h:102
@@ -98,3 +101,3 @@
 
   uint32_t getEdgeWeight(const BasicBlock *Src,
                          succ_const_iterator Dst) const;
----------------
Same here.

================
Comment at: include/llvm/Analysis/BranchProbabilityInfo.h:111
@@ -107,3 +110,3 @@
   /// weights are calculated carefully before using!
   void setEdgeWeight(const BasicBlock *Src, unsigned IndexInSuccessors,
                      uint32_t Weight);
----------------
Same here. I don't see a a reason to keep both Probability and weight interfaces.

(the only place weight is useful is branch meta data)

================
Comment at: include/llvm/Analysis/BranchProbabilityInfo.h:138
@@ -129,3 +137,3 @@
 
   DenseMap<Edge, uint32_t> Weights;
 
----------------
DenseMap<Edge, BranchProbability> Probabilities;

================
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);
----------------
A side note -- I remember you mention in if conversion, duplicate cfg edges do exist?

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:433
@@ +432,3 @@
+  /// to keep their relative relations.
+  void addSuccessor(MachineBasicBlock *Succ);
+
----------------
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.

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:732
@@ -717,3 +731,3 @@
   /// MachineBranchProbabilityInfo class.
   uint32_t getSuccWeight(const_succ_iterator Succ) const;
 
----------------
Remove this method.

================
Comment at: include/llvm/Support/BranchProbability.h:41
@@ -40,3 +40,3 @@
   BranchProbability() : N(0) {}
   BranchProbability(uint32_t Numerator, uint32_t Denominator);
 
----------------
Any reason why Numerator and Denominator can not be uint64_t?

================
Comment at: include/llvm/Support/BranchProbability.h:45
@@ -44,3 +44,3 @@
 
   static BranchProbability getZero() { return BranchProbability(0); }
   static BranchProbability getOne() { return BranchProbability(D); }
----------------
It is useful to introduce a static helper method (templatized) to return a default BranchProbability 

template<class BlockT> BranchProbability getDefaultProbabillity(BlockT *);

================
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;
----------------
when can overflow happen? Also should the check really be:

(uint64_t)N + RHS.H > D ?

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

================
Comment at: lib/CodeGen/MIRPrinter.cpp:465
@@ -466,1 +464,3 @@
+      if (MBB.hasSuccessorProbabilities())
+        OS << '(' << MBB.getSuccProbability(I) << ')';
     }
----------------
Avoid using the private method getSuccProbability here?

================
Comment at: lib/CodeGen/MachineBranchProbabilityInfo.cpp:44
@@ +43,3 @@
+  auto Prob = Src->getSuccProbability(Dst);
+  if (Prob.isZero())
+    return BranchProbability(1, Src->succ_size());
----------------
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.

================
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);
----------------
Use a helper method for this (getDefaultProbability)


http://reviews.llvm.org/D13745