[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:526
@@ +525,3 @@
+  // probability list, so we fill all Probs with 0's.
+  if (!Prob.isZero() && Probs.empty())
+    Probs.resize(Successors.size());
----------------
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

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:529
@@ +528,3 @@
+
+  if (!Prob.isZero() || !Probs.empty())
+    Probs.push_back(Prob);
----------------
When Probs.empty() is false, shall we assert that Prob must be known?

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:551
@@ +550,3 @@
+    probability_iterator WI = getProbabilityIterator(I);
+    Probs.erase(WI);
+  }
----------------
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.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:617
@@ +616,3 @@
+  if (!Probs.empty()) {
+    probability_iterator OldWI = getProbabilityIterator(OldI);
+    *getProbabilityIterator(NewI) += *OldWI;
----------------
oldWI --> oldPI

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:618
@@ +617,3 @@
+    probability_iterator OldWI = getProbabilityIterator(OldI);
+    *getProbabilityIterator(NewI) += *OldWI;
+    Probs.erase(OldWI);
----------------
why += not = ?

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1153
@@ -1118,1 +1152,3 @@
 
+/// Return probability of the edge from this block to MBB.
+BranchProbability
----------------
document that default probability is returned when there is no information.  



================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1172
@@ +1171,3 @@
+                                           BranchProbability Prob) {
+  if (Probs.empty())
+    return;
----------------
Why the behavior here is different from addSuccessor where only 'unknown' probs are ignored.


http://reviews.llvm.org/D13908