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

Cong Hou via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 9 11:42:45 PST 2015


congh marked an inline comment as done.
congh added a comment.

In http://reviews.llvm.org/D14361#284713, @davidxl wrote:

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


There are passes like IfConversion, BranchFolding, MachineBlockPlacement, TailDuplication, and MIParser that are using those interfaces. This patch is our planned step two, in which only SelectionDAGBuilder is using the new interfaces. Maybe we want to combine step 2 and 3?


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

================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:565
@@ -556,1 +564,3 @@
+  // passes. Uncomment it once all edge weights are replaced with probabilities.
+#if 0
   // If probability list is empty it means we don't use it (disabled
----------------
manmanren wrote:
> I don't quite get why we need to comment out this. When we support both weights and branch probabilities, should we keep both? We can remove weights in the end, right?
As the BP-based new interfaces are now only used in SelectionDAGBuilder, and later passes are still using old weight-based interfaces, it is possible that a later pass adds a successor with only weight and then removes this successor, which doesn't have any probability stored. Yes, we will remove all weight-based interfaces in the end.

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

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

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

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

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:8425
@@ -8426,1 +8424,3 @@
+  thisMBB->addSuccessor(mainMBB, /* probability */ {1, 100});
+  thisMBB->addSuccessor(sinkMBB, /* probability */ {99, 100});
 
----------------
manmanren wrote:
> This change seems to be different from the weight distribution before. Can you add comments on why?
This is due to an old assumption that the edge weight should not be zero. Actually there is a bug here: the zero weight will be converted to a default weight 16 later. But now as we can use zero probability, I will update it by passing 0 and 1 probability.


http://reviews.llvm.org/D14361





More information about the llvm-commits mailing list