[PATCH] D30631: [BPI] Use metadata info before any other heuristics

[Serguei Katkov via Phabricator via llvm-commits]

skatkov marked an inline comment as done.
skatkov added inline comments.


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:254
   const TerminatorInst *TI = BB->getTerminator();
-  if (TI->getNumSuccessors() == 1)
+  if (TI->getNumSuccessors() <= 1)
     return false;
----------------
chandlerc wrote:
> Didn't this get factored out into a separate patch? Not a big deal, but seems like a clear thing to factor out.
It will be in the next patch which you have already reviewed but I made that patch to depend on this one, so I will handle it after this patch is landed.


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:330-341
+  // If we modified the probability of some edges then we must distribute
+  // the difference between reachable blocks.
+  if (ToDistribute > BranchProbability::getZero()) {
+    assert(ReachableIdxs.size() && "Must be at least one reachable successor");
+    BranchProbability PerEdge = ToDistribute / ReachableIdxs.size();
+    for (auto i : ReachableIdxs) {
+      BP[i] += PerEdge;
----------------
chandlerc wrote:
> Lift all of this into the if for there being some unreachable and some reachable successors? Just seems worth skipping the ToDistribute checks in the case where none of this matters.
ok


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:337
+      BP[i] += PerEdge;
+      ToDistribute -= PerEdge;
+    }
----------------
chandlerc wrote:
> Is it better to do this in the loop or to multiply by size and subtract that once? It seems simpler to write the latter way inside the addition below:
> 
>   BP[ReachableIdxs[0]] += ToDistribute - (PerEdge * ReachableIdxs.size());
Will do.


https://reviews.llvm.org/D30631