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

[Cong Hou via llvm-commits]

congh marked 2 inline comments as done.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:526
@@ +525,3 @@
+  // probability list, so we fill all Probs with 0's.
+  if (!Prob.isZero() && Probs.empty())
+    Probs.resize(Successors.size());
----------------
davidxl wrote:
> zero is a valid probability, so do not use to signal 'unknown'
> 
> I think we should define a special const BP value to represent 'unknown' BP (which is different from default 1/N). This is doable as the our choice of denominator is 1<<31, so a larger than 1<<1 nominator can be picked to represent the special reserved value
Right. I have created such an unknown probability by using UINT32_MAX as the numerator.

I think we can always keep unknown probabilities in the list and when getSuccProb() is called to get the probability of a successor, and the returned probability is unknown, we can calculate a "default" value on the fly, which is (1-P) / N, where P is the sum of all known probabilities, and N is the number of unknown probabilities. What do you think?

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:529
@@ +528,3 @@
+
+  if (!Prob.isZero() || !Probs.empty())
+    Probs.push_back(Prob);
----------------
davidxl wrote:
> When Probs.empty() is false, shall we assert that Prob must be known?
If we want to use the unknown probability to represent a "default" probability then we cannot make this assertion here. Unless we create another interface which is used to add successor when the probability is unknown (which is not added here as it will conflict with the one that already exists with second argument having a default value). I think a good idea is this interface should be always called with a known probability.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:551
@@ +550,3 @@
+    probability_iterator WI = getProbabilityIterator(I);
+    Probs.erase(WI);
+  }
----------------
davidxl wrote:
> normalization needed or it is premature to do normalization now? It is probably the later as this is low level interface with very little context.
Yes, it seems the normalization should be called explicitly by the user of this interface. One way to validate BPI is check if the sum of successors' probabilities is around 1 for all blocks, so we can get warnings if the user forget to normalize probabilities. 

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:618
@@ +617,3 @@
+    probability_iterator OldWI = getProbabilityIterator(OldI);
+    *getProbabilityIterator(NewI) += *OldWI;
+    Probs.erase(OldWI);
----------------
davidxl wrote:
> why += not = ?
Here New is already a successor and this function behaves like merging two successors into one. That is why += is used.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1172
@@ +1171,3 @@
+                                           BranchProbability Prob) {
+  if (Probs.empty())
+    return;
----------------
davidxl wrote:
> Why the behavior here is different from addSuccessor where only 'unknown' probs are ignored.
I have added an assertion checking Prob is not unknown. We should not allow unknown probability to be used here.


http://reviews.llvm.org/D13908