[PATCH] D15519: Replace weights by probabilities in BPI.

[Cong Hou via llvm-commits]

congh added inline comments.

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:143
@@ -149,9 +142,3 @@
 
-  uint32_t UnreachableWeight =
-    std::max(UR_TAKEN_WEIGHT / (unsigned)UnreachableEdges.size(), MIN_WEIGHT);
-  for (SmallVectorImpl<unsigned>::iterator I = UnreachableEdges.begin(),
-                                           E = UnreachableEdges.end();
-       I != E; ++I)
-    setEdgeWeight(BB, *I, UnreachableWeight);
-
-  if (ReachableEdges.empty())
+  if (ReachableEdges.empty()) {
+    BranchProbability Prob(1, UnreachableEdges.size());
----------------
davidxl wrote:
> Can we unify the code a little more:
> 
> BranchProbability UnreachableProb =
>    ( ReachableEdges.empty() ?
>            BranchProbability(1, UnreachableEdges.size()) :
>            BranchProbability(UR_TAKEN_WEIGHT, (....) * UnreachableEdges.size()));
> BranchProbability ReachableProb = (UR_NOTAKEN_WEIGHT, ReachableEdges.size() * ...);
> 
> for (...: UnreachableEdges) 
>     set...
> for (...)
>     set ...
> 
OK. We can set UnreachableProb to 100% when ReachableEdges is empty.

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:264
@@ -267,9 +263,3 @@
 
-  uint32_t ColdWeight =
-      std::max(CC_TAKEN_WEIGHT / (unsigned) ColdEdges.size(), MIN_WEIGHT);
-  for (SmallVectorImpl<unsigned>::iterator I = ColdEdges.begin(),
-                                           E = ColdEdges.end();
-       I != E; ++I)
-    setEdgeWeight(BB, *I, ColdWeight);
-
-  if (NormalEdges.empty())
+  if (NormalEdges.empty()) {
+    BranchProbability Prob(1, ColdEdges.size());
----------------
davidxl wrote:
> Similarly, the code can be simplified without special case..
Done.

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:347
@@ +346,3 @@
+  SmallVector<BranchProbability, 4> Probs(3, BranchProbability::getZero());
+  if (!BackEdges.empty())
+    Probs[0] = BranchProbability(LBH_TAKEN_WEIGHT,
----------------
davidxl wrote:
> No need to guard.
This is needed to keep the original behavior: when BackEdges is empty, we don't want to normalize probabilities including the BP in this branch. Part of the reason is that we don't have a function to normalize all BB's successors' probabilities (although we have one for MBB).

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:350
@@ +349,3 @@
+                                 LBH_TAKEN_WEIGHT + LBH_NONTAKEN_WEIGHT);
+  if (!InEdges.empty())
+    Probs[1] = BranchProbability(LBH_TAKEN_WEIGHT,
----------------
davidxl wrote:
> No need to guard.
Ditto.

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:353
@@ +352,3 @@
+                                 LBH_TAKEN_WEIGHT + LBH_NONTAKEN_WEIGHT);
+  if (!ExitingEdges.empty())
+    Probs[2] = BranchProbability(LBH_NONTAKEN_WEIGHT,
----------------
davidxl wrote:
> No need to guard.
Ditto.

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:361
@@ -353,1 +360,3 @@
+    for (unsigned SuccIdx : BackEdges)
+      setEdgeProbability(BB, SuccIdx, Prob);
   }
----------------
davidxl wrote:
> The original code will adjust the weight if it is too low. Do we need to check Prob here too?
I think this is unnecessary: the weight can become too small (like zero) after being divided but we don't have this issue in probabilities.

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:586
@@ -599,5 +585,3 @@
     if (*I == Dst) {
-      MapI = Weights.find(std::make_pair(Src, I.getSuccessorIndex()));
-      if (MapI != Weights.end()) {
-        FoundWeight = true;
-        Weight += MapI->second;
+      auto MapI = Probs.find(std::make_pair(Src, I.getSuccessorIndex()));
+      if (MapI != Probs.end()) {
----------------
davidxl wrote:
> Is it better just call getEdgeProbability(Src, *I)
We need to take care of the case MapI == Probs.end() so we cannot call getEdgeProbability(Src, *I).


http://reviews.llvm.org/D15519