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

[David Li via llvm-commits]

davidxl added a comment.

How many more weight based uses are still remaining after this patch? If there are only a few, consider move those uses into this patch too. It is a weird state that both weights and probs exists (and used at  the same time).


================
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;
----------------
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.

================
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);
----------------
Use a helper interface in MachineBasicBlock?

================
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.
----------------
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.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1835
@@ +1834,3 @@
+    // TrueBB and FalseBB are always different unless the incoming IR is
+    // degenerate. This only happens when running llc on weird IR.
+
----------------
can you add more comments describing the lowered CFG and explain why the profile is updated this way?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1842
@@ +1841,3 @@
+    if (NormalizedProbs[0].isZero())
+      NormalizedProbs[0] = getEdgeProbability(SwitchBB, CB.TrueBB);
+    if (NormalizedProbs[1].isZero())
----------------
Why can CB.TrueProb be zero and be different from getEdgeProbability(SwitchBB, CB.TrueBB) ?

================
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);
+
----------------
Why they don't sum up to one and needs normalization? Is the problem in the caller?


http://reviews.llvm.org/D14361