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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/Support/BranchProbability.h:95
@@ -97,1 +94,3 @@
+    // Saturate the result in case of overflow.
+    N = (uint64_t(N) + RHS.N > D) ? D : N + RHS.N;
     return *this;
----------------
congh wrote:
> davidxl wrote:
> > Saturation due to rounding errors is fine, but it seems we may need a way to differentiate the cases where true user errors we made.
> So is it better to remove the rounding behavior when constructing BP?
you mean always round to zero? probably not a good idea.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1499
@@ +1498,3 @@
+    // the number of successors.
+    auto SuccSize = std::max<uint32_t>(
+        std::distance(succ_begin(SrcBB), succ_end(SrcBB)), 1);
----------------
congh wrote:
> davidxl wrote:
> > Use a helper interface in MachineBasicBlock?
> SrcBB is a BB not a MBB. I think we don't want to change BB right?
Can probably be added when weight based interrfaces for BB are changed later.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1613
@@ -1617,1 +1612,3 @@
+    // BB1's probabilities to A/2 and A/2+B, and set TmpBB's probabilities to
+    // A/(1+B) and 2B/(1+B). This choice assumes that
     //   TrueProb for BB1 == FalseProb for BB1 * TrueProb for TmpBB.
----------------
congh wrote:
> davidxl wrote:
> > The Prob distribution for TemBB does not look correct --- they don't sum up to 1.
> > 
> > It should be A/(A+2B), 2B/(A+2B).
> > 
> > It is probably useful to introduce probability division interface.
> As A+B=1, A/(1+B) + 2B(1+B) = (A+2B)/(1+B) = (1+B)/(1+B)=1.
> 
> I doubt if the division between BP that is defined in BP is useful: we may frequently get a result that is larger than one. Fortunately, it seems we may only need this operation here.
You are right -- A/(A+2B)  actually = A/(A+B +B ) = A/(1+B), so your is correct.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1624
@@ +1623,3 @@
+
+    NewTrueProb = BranchProbability(
+        TProb.getNumerator(),
----------------
Would it be better to use a new variable TmpTrueProb here? (similarly TmpFalseProb)?

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

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1660
@@ +1659,3 @@
+
+    NewTrueProb = BranchProbability(
+        uint64_t(2) * TProb.getNumerator(),
----------------
Similarly here.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1842
@@ +1841,3 @@
+    if (NormalizedProbs[0].isZero())
+      NormalizedProbs[0] = getEdgeProbability(SwitchBB, CB.TrueBB);
+    if (NormalizedProbs[1].isZero())
----------------
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?

================
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);
+
----------------
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?


http://reviews.llvm.org/D14361