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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Sorry I couldn't get back to it sooner, first chance I had.

However, this looks really, really nice. Thanks for seeing it all the way through. I love the test cases where we nicely zero out the unreachable bits of the switch but leave the clear hot path based on metadata.

Some really minor code suggestions below. Feel free to land with those.



================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:254
   const TerminatorInst *TI = BB->getTerminator();
-  if (TI->getNumSuccessors() == 1)
+  if (TI->getNumSuccessors() <= 1)
     return false;
----------------
Didn't this get factored out into a separate patch? Not a big deal, but seems like a clear thing to factor out.


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


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:337
+      BP[i] += PerEdge;
+      ToDistribute -= PerEdge;
+    }
----------------
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());


https://reviews.llvm.org/D30631