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

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:529
@@ +528,3 @@
+  }
+
+  if (!Probs.empty()) {
----------------
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?

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1164
@@ +1163,3 @@
+  if (Probs.empty())
+    return BranchProbability(1, succ_size());
+
----------------
congh wrote:
> davidxl wrote:
> > Is it up to the user to decide whether a default value should be returned? 
> Should the user know that the returned probability is an unknown one?  It is like an undefined behavior and here we just give it a definition.
Ok -- i guess the direct caller of this function does not have any better info.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1167
@@ +1166,3 @@
+  auto Prob = *getProbabilityIterator(Succ);
+  if (Prob.isUnknown()) {
+    uint32_t UnknownProbNum = 0;
----------------
congh wrote:
> davidxl wrote:
> > How is this possible? Should be it asserted that Prob is Known?
> As I suggested before, we can keep all unknown probabilities (which is used to represent default probabilities) in MBB and return a "default" value on the fly.
See my comments above. If we don't ever mix unknown and known BP, there is no need to store any unknown BPs.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1191
@@ +1190,3 @@
+                                           BranchProbability Prob) {
+  assert(!Prob.isUnknown());
+  if (Probs.empty())
----------------
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?


http://reviews.llvm.org/D13908