[PATCH] D14361: Let SelectionDAG start to use probability-based interface to add successors.

[Cong Hou via llvm-commits]

congh added inline comments.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:551
@@ -544,3 +550,3 @@
 
 void MachineBasicBlock::removeSuccessor(MachineBasicBlock *Succ) {
   Succ->removePredecessor(this);
----------------
davidxl wrote:
> Can you refactor this function let it call the iterator based removeSuccessor (in a different patch)?
OK. I will let this function call iterator based removeSuccessor().

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:587
@@ -575,1 +586,3 @@
 
+  // FIXME: Temporarily comment the following code as probabilities are now only
+  // used during instruction lowering, but this interface is called in later
----------------
davidxl wrote:
> Is it better to relax getProbabilityIterator for now to not assert and return end iterator instead?
> 
> Also can the opposite situation happen? The removed old BB does not have an corresponding weight? In that case, it is better to relax the getXXXiterator for now.
> Is it better to relax getProbabilityIterator for now to not assert and return end iterator instead?

I think the assertion doesn't hurt and can help us capture exceptions. What is the advantage of using an end iterator? You mean to eliminate some branches?

> 
> Also can the opposite situation happen? The removed old BB does not have an corresponding weight? In that case, it is better to relax the getXXXiterator for now.

This should not happen: we should always guarantee that the weight list is either empty or has the same size of the successor list.

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:635
@@ -618,3 +634,3 @@
   // Update its weight instead of adding a duplicate edge.
   if (!Weights.empty()) {
     weight_iterator OldWI = getWeightIterator(OldI);
----------------
davidxl wrote:
> The refactored removeSuccessor can call an internal function
> 
> removeSuccessor(succ_iterator OldI, succ_iterator NewI) -- when NewI is end(), do not update NewI's weight/prob.
> 
> replaceSuccessor can then call this method as well.
Do you mean replaceSuccessor? If New is not added to the successor list, how to reference it with an iterator?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1632
@@ -1632,3 +1631,3 @@
     FindMergedConditions(BOp->getOperand(1), TBB, FBB, TmpBB, SwitchBB, Opc,
-                         NewTrueWeight, NewFalseWeight);
+                         NewTrueProb, NewFalseProb);
   } else {
----------------
davidxl wrote:
> I think it will be very useful if you can introduce an interface like this (a public static member of BranchProbability) f0r the most common case:
> 
> void getNormalizedProbabilities(BranchProbability& TrueProb, BranchProbability& FalseBB);
> 
> With this interface, the low level manipulation of numerator and denominator can be eliminated and the code becomes more readable.
> 
> Note that one of the benefits of using BP interface is it helps reasoning in updating -- so we should definitely avoid doing the low level synthesis above.
> 
> 
OK, this sounds good to me. In this case as 2*B may exceed one, we can obtain A/(1+B) and 2B/(1+B) by calling getNormalizedProbabilities(A/2, B). I will update the patch accordingly.


http://reviews.llvm.org/D14361