[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/SelectionDAG/SelectionDAGBuilder.cpp:1624
@@ +1623,3 @@
+
+    NewTrueProb = BranchProbability(
+        TProb.getNumerator(),
----------------
davidxl wrote:
> Would it be better to use a new variable TmpTrueProb here? (similarly TmpFalseProb)?
I think it should have similar name as NewTrueProb like NewTrueProb2 so that it is obvious that we are updating two related branches's probabilities in parallel. 

================
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:
> It seems more readable to set TmpBB's prob to {A/2, B}, and then later normalize it using the normalization interface.
{A/2, B} may exceed 1 so this may not be applicable.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1842
@@ +1841,3 @@
+    if (NormalizedProbs[0].isZero())
+      NormalizedProbs[0] = getEdgeProbability(SwitchBB, CB.TrueBB);
+    if (NormalizedProbs[1].isZero())
----------------
davidxl wrote:
> congh wrote:
> > davidxl wrote:
> > > Why can CB.TrueProb be zero and be different from getEdgeProbability(SwitchBB, CB.TrueBB) ?
> > Previously, CB.TrueWeight can also be zero, in which case getEdgeWeight() is also called, which is wrapped in addSuccessorWithWeight. As now we have normalizeSuccProbs() in MBB, I have update this part by using it.
> Is it safe to unconditionally use getEdgeProbability()? will it return different value?
No. CB.TrueProb is precalculated in this file, but in some cases it can be zero, which means it is not calculated at all. In this case we use its original value from BB.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1847
@@ +1846,3 @@
+    // Normalize probabilities of true/false body so that their sum equals one.
+    BranchProbability::normalizeProbabilities(NormalizedProbs);
+
----------------
davidxl wrote:
> congh wrote:
> > davidxl wrote:
> > > Why they don't sum up to one and needs normalization? Is the problem in the caller?
> > The calculation of  CB.TrueProb and CB.FalseProb cannot guarantee that their sum is one. The existed code is for calculating weights which only keeps correct relative relations of CB.True/FalseProb. And in this file there are several other places in which we need to normalize a group of probabilities.
> Can you point out the those lines in the file that assign non-normalized PBs?
You can search normalizeSuccProbs and will find three uses in this file.


http://reviews.llvm.org/D14361