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

[Manman Ren via llvm-commits]

manmanren added a comment.

Thanks for working on this!

This time, I managed to look through all the changes. The math looks right!


================
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;
----------------
If this change is not required for replacing weight-based with probability-based, please commit it separately.

================
Comment at: include/llvm/Support/BranchProbability.h:144
@@ +143,3 @@
+  if (Probs.empty())
+    return;
+
----------------
Please commit this separately.

================
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);
   }
----------------
Do we need to normalize the probs after calling addSuccessorWithProb?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2049
@@ -2053,1 +2048,3 @@
+  addSuccessorWithProb(SwitchBB, B.Default, B.DefaultProb);
+  addSuccessorWithProb(SwitchBB, MBB, B.Prob);
 
----------------
Do we need to normalize the probs after calling addSuccessorWithProb?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8166
@@ -8163,1 +8165,3 @@
+        else
+          addSuccessorWithProb(SwitchMBB, DefaultMBB);
 
----------------
Should we call addSuccessorWithoutProb when BPI does not exist?

================
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;
 
----------------
Will this exceed 1? Have to admit that I didn't really look into this.


http://reviews.llvm.org/D14361