[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.

Thanks for the review!

In http://reviews.llvm.org/D13745#268551, @manmanren wrote:

> I would like to see this patch being separated into smaller ones if possible. We can keep both addSuccessor(..., Weight) and addSuccessor(..., BranchProbability) for a short period of time to ease the transition.


OK, this sounds good. I will produce another patch that keeps both interfaces.

> I am mostly concerned about the changes in updating branch weights/probs during transformation (SelectionDAGBuilder, BranchFolding, IfConversion etc). If we can separate the changes, it will be much easier to review and to debug later on if we have a performance issue.


Sounds good.


================
Comment at: include/llvm/Analysis/BranchProbabilityInfo.h:121
@@ +120,3 @@
+    return IsLikely ? LikelyProb : LikelyProb.getCompl();
+  }
+
----------------
manmanren wrote:
> Should we remove getBranchWeightStackProtector after getBranchProbStackProtector is added?
It is still used in StackProtector to build MDNode.

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:427
@@ -425,3 +426,3 @@
   /// Note that duplicate Machine CFG edges are not allowed.
-  void addSuccessor(MachineBasicBlock *Succ, uint32_t Weight = 0);
+  void addSuccessor(MachineBasicBlock *Succ, BranchProbability Prob);
 
----------------
manmanren wrote:
> Can you comment on if the sum of probabilities for all successors is 1, after adding the successor?
OK. The answer is no. This interface will be called several times when adding many successors so during any intermediate state the sum of probabilities of all successors is not 1. I will state it clearly in the comment.

================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:433
@@ +432,3 @@
+  /// to keep their relative relations.
+  void addSuccessor(MachineBasicBlock *Succ);
+
----------------
congh wrote:
> 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.
When we don't pass a branch probability when adding a successor, we don't have enough information or intentionally want to use a "default" probability. Here we use 1/n as a "default" probability, where n is the new number of successors. To make sure this works, we need to normalize the probabilities of all successors.

================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:462
@@ -461,3 +461,3 @@
     }
-    MBB.addSuccessor(SuccMBB, Weight);
+    MBB.addSuccessor(SuccMBB, BranchProbability(Prob, 100));
   } while (consumeIfPresent(MIToken::comma));
----------------
manmanren wrote:
> 
> Can you add a comment for using 100? Because it is printed out as percentile?
> It is kind of weird to print out as (0xbb / 0xbb = 33.0%), and to parse as (33).
> Can you round-trip the printout?
OK. Yes, this should be fixed buy using the percentage representation in print.


http://reviews.llvm.org/D13745