[PATCH] D13908: Add new interfaces to MBB for manipulating successors with probabilities instead of weights.

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:527
@@ +526,3 @@
+  if (!Prob.isUnknown() && Probs.empty()) {
+    Probs.assign(Successors.size(), BranchProbability::getUnknown());
+  }
----------------
congh wrote:
> davidxl wrote:
> > congh wrote:
> > > davidxl wrote:
> > > > I think it is better to assert Successors.size() is 0 here. Under what situation can have mixed 'known' and 'known' probabilities?
> > > As we have concluded before, currently the unknown weight and default weight are both added through the same interface, and it is impossible to differentiate them. I suggest for addSuccessor() of weight version we can split it into two interfaces:
> > > 
> > > 1. addSuccessorWithWeight(MBB *succ, uint32_t weight = 0);
> > > 
> > > This one is used when the weight is known, and we use 0 as a default weight. When this interface is called, the weight list should always have the same size of successor list.
> > > 
> > > 2. addSuccessor(MBB *succ);
> > > 
> > > This one is used when the weight is unknown (BPI is unavailable). When this interface is called, the weight list should always be empty.
> > > 
> > > We can introduce similar interfaces for probability version.
> > > 
> > Can you elaborate on why two use cases can not be differentiated? It seems that the unknown BP should only be used when BPI == 0 (i.e. when Optimization level == 0).
> Consider this scenario: we first add a successor with a known (precalculated) probability 1/2, then add another successor with the default probability. Now as N=2, it is assigned with 1/2. Then we add the third successor with the default probability. Now the sum of the other two successors' probabilities is already 1, so we can normalize them. But we could not recover the probability of the first successor to 1/2 anymore, which is a precalculated one and should not be changed later. This is the tricky situation. If we record those two default probabilities as unknown, we could calculated them on the fly (then the probabilities of three successors will be 1/2, 1/4, 1/4, which is more reasonable), and everything will be ok.
In the long run, we should fix those client code that provides default probability as many as we can. As it stands today, it is garbage-in and garbage-out situation -- we don't need to pretend one handling is better than the other. 

The profile sanity tool we plan to have in the future is supposed to detect problems like this.


http://reviews.llvm.org/D13908