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


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1164
@@ +1163,3 @@
+  if (Probs.empty())
+    return BranchProbability(1, succ_size());
+
----------------
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.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1167
@@ +1166,3 @@
+  auto Prob = *getProbabilityIterator(Succ);
+  if (Prob.isUnknown()) {
+    uint32_t UnknownProbNum = 0;
----------------
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.

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


http://reviews.llvm.org/D13908