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

Cong Hou via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 17 17:54:57 PST 2015


congh marked 3 inline comments as done.
congh added a comment.

In http://reviews.llvm.org/D14361#291372, @manmanren wrote:

> Thanks for working on this!
>
> This time, I managed to look through all the changes. The math looks right!


Thank you for the review!

For the safety sake, I am wondering if we should normalize successors' probabilities for all MBB in the CFG after ISel is done. Then we can safely remove those individual normalizations in SelectionDAGBuilder.cpp.


================
Comment at: include/llvm/Support/BranchProbability.h:96
@@ -97,1 +95,3 @@
+    // Saturate the result in case of overflow.
+    N = (uint64_t(N) + RHS.N > D) ? D : N + RHS.N;
     return *this;
----------------
OK. Done.

================
Comment at: include/llvm/Support/BranchProbability.h:144
@@ +143,3 @@
+void BranchProbability::normalizeProbabilities(ProbabilityIter Begin,
+                                               ProbabilityIter End) {
+  if (Begin == End)
----------------
Done.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1314
@@ -1313,3 +1313,3 @@
     UnwindDest.first->setIsEHPad();
-    addSuccessorWithWeight(FuncInfo.MBB, UnwindDest.first, UnwindDest.second);
+    addSuccessorWithProb(FuncInfo.MBB, UnwindDest.first, UnwindDest.second);
   }
----------------
I think yes, here we need to normalize probabilities. I checked the function that calculates them and found we cannot guarantee the sum of them is 1. Thanks for pointing it out!

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2049
@@ -2053,1 +2048,3 @@
+  addSuccessorWithProb(SwitchBB, B.Default, B.DefaultProb);
+  addSuccessorWithProb(SwitchBB, MBB, B.Prob);
 
----------------
I think the answer is yes again here. I am wondering if we should always do it as long as we are uncertain whether the sum of successors's probabilities is one. This of cause needs potential redundant calculations but it will be safer.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8166
@@ -8163,1 +8165,3 @@
+        else
+          addSuccessorWithProb(SwitchMBB, DefaultMBB);
 
----------------
addSuccessorWithProb() will take care of it, in which addSuccessorWithoutProb() is called if BPI is not available.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8352
@@ -8351,1 +8351,3 @@
+  auto LeftProb = LastLeft->Prob + W.DefaultProb / 2;
+  auto RightProb = FirstRight->Prob + W.DefaultProb / 2;
 
----------------
It should not exceed 1: LastLeft->Prob or FurstRight->Prob is the probability of a case statement and W.DefaultProb is the probability of the default statement. 


http://reviews.llvm.org/D14361





More information about the llvm-commits mailing list