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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

First off, thanks for the new approach. I like this direction a lot. Some more tactical comments here.



================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:299-304
+    auto UnreachableProb =
+        UnreachableCount == TI->getNumSuccessors()
+            ? BranchProbability::getBranchProbability(1, TI->getNumSuccessors())
+            : BranchProbability::getBranchProbability(
+                  UR_TAKEN_WEIGHT,
+                  (UR_TAKEN_WEIGHT + UR_NONTAKEN_WEIGHT) * UnreachableCount);
----------------
There is a lot of code here. I wonder, is it possible to share the logic here with the logic above that is used in the absence of metadata?


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:306
+    for (unsigned i = 0, e = TI->getNumSuccessors(); i != e; ++i) {
+      if (!PostDominatedByUnreachable.count(TI->getSuccessor(i)))
+        continue;
----------------
To avoid re-hitting this set for every successor, you could above append the successor indices that are in this set to a list, and then loop over that list here. The size of the list would still give you the count of unreachable successors vs. reachable.


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:308-311
+      auto MetadataProb = BranchProbability::getBranchProbability(
+          Weights[i], static_cast<uint32_t>(WeightSum));
+      if (UnreachableProb < MetadataProb)
+        return false;
----------------
I feel like it would be nicer to just adjust the weight downward such that the probability is essentially the minimum of the two sources of information. That way we don't lose the metadata's weights for the different successors that *don't* go to unreachable.

Consider the test case (in pseudo C code):

  for (...) {
    switch (cond) {
      default:
        unreachable
      case 2: // HOT
        // something tiny
        continue;
      case 3: // COLD
        // huge pile of ugly code
        continue;
      }
    }
  }

If, for whatever reason, we end up with one sample in the metadata going to unreachable, we'll completely loose the metadata that distinguishes between hot and cold here.

Does that make sense?


https://reviews.llvm.org/D30631