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

[Cong Hou via llvm-commits]

congh added inline comments.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:527
@@ +526,3 @@
+  if (!Prob.isUnknown() && Probs.empty()) {
+    Probs.assign(Successors.size(), BranchProbability::getUnknown());
+  }
----------------
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.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:529
@@ +528,3 @@
+  }
+
+  if (!Probs.empty()) {
----------------
davidxl wrote:
> For the new interfaces, I think we should enforce the policy:
> 1) do not mix unknown and known branch probabilities
> 2) unknown branch probability is only used (in constructor/setter) when optimization is disabled. 
> 3) when optimization is not disabled, known BP should be used always -- even though there exist client code that does not provide the information -- the implementation can choose to use the 1/N default
> 
> 4) the public BP getter interfaces should never return unknown BP (private ones can if needed)
> 
> How does that sound?
Please check my example above. However, I need to confirm that that situation happens. I could make a patch to differentiate addSuccessor() with known/default weights and with unknown weights, and then check if calls of this function have mixed known and default weights.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1191
@@ +1190,3 @@
+                                           BranchProbability Prob) {
+  assert(!Prob.isUnknown());
+  if (Probs.empty())
----------------
davidxl wrote:
> congh wrote:
> > davidxl wrote:
> > > my question is addSuccessor interface will allocate Prob vector if Prob is known, but the behavior here is different.
> > When setSuccProbability() is called, the user must have known a known probability otherwise why he would like to call this function? That is why we need to check that the passed in probability is known.
> but why return when Prob is empty?
The input is an iterator and when prob list is empty, we can do nothing (we cannot find the corresponding BP for the given successor). setEdgeWeight has the similar behavior.
So you are thinking about the case that all successors have default weights and then the weight list can be empty, right? I think this could happen... and this is why we need to follow the rule that the weight list is empty only if BPI is unavailable.


http://reviews.llvm.org/D13908