[PATCH] D36864: [Profile] backward propagate profile data in jump-threading

[Chandler Carruth via Phabricator via llvm-commits]

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

Awesome. LGTM with the minor things below addressed. Let me know if any of this isn't make sense.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:194
+
+    BP = (CI->getZExtValue() ? BranchProbability::getBranchProbability(
+                                   TrueWeight, TrueWeight + FalseWeight)
----------------
davidxl wrote:
> chandlerc wrote:
> > This code only handle i1 PHIs that directly feed conditional branches, right?
> > 
> > Provided that's so, I would assert that CI is a 1 bit integer here, and then test whether it the null value or not rather than zexting and extracting it.
> There is an early check of one bit type. I am not clear what is the suggestion about (regarding extracting value).
Essentially...

  assert(CI.getBitWidth() == 1 && "We only handle i1 constants here.");
  BP = CI->isOne() ? ... : ...;

You could also use `CI->isZero()`, but one seems more clear.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:208-209
+    // FIXME: we currently only set the profile data when it is missing.
+    // With PGO, this can be turned on too to refine the profile with
+    // context information.
+    if (PredBr->extractProfMetadata(PredTrueWeight, PredFalseWeight))
----------------
chandlerc wrote:
> "With PGO, this can be turned on too to refine the profile with" -> "With PGO, this can be used to refine even existing profile data with"
This wording improvement for the comment would still be helpful I think.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:163
+//  can not be greater than 1%.
+
+static void updatePredecessorProfileMetadata(PHINode *PN, BasicBlock *BB) {
----------------
nit: spurious blank line...


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:176
+  // that leads to the PhiNode's incoming block:
+  auto GetPredOutEdge = [](BasicBlock *IncomingBB, BasicBlock *PhiBB) -> std::pair<BasicBlock *, BasicBlock *> {
+    auto *PredBB = IncomingBB;
----------------
nit: clang-format to fix line length


https://reviews.llvm.org/D36864