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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Really, really nice find here. A bunch of comments below, but they're all pretty minor on the whole.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:159
+//  can not be greater than 1%.
+
+static void UpdatePredecessorProfileMetadata(PHINode *PN, BasicBlock *BB) {
----------------
Would be good to connect this comment to the arguments and behavior of this function.

(Don't get me wrong, this comment is *excellent* for explaining why we can do this back propagation and how to compute it! I just also want comments that explain how this routine fits into the model.)


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:160
+
+static void UpdatePredecessorProfileMetadata(PHINode *PN, BasicBlock *BB) {
+  BranchInst *CondBr = dyn_cast<BranchInst>(BB->getTerminator());
----------------
Update -> update


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:175
+    if (PredBr && PredBr->isConditional())
+      return std::make_pair(IncomingBB, PhiBB);
+    auto *PredBB = IncomingBB->getSinglePredecessor();
----------------
You can use `return {IncomingBB, PhiBB};` here and elsewhere to return. You'll have to specify the return type in the lambda, but then you won't even need to do that for nullptrs...


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:176-177
+      return std::make_pair(IncomingBB, PhiBB);
+    auto *PredBB = IncomingBB->getSinglePredecessor();
+    if (!PredBB)
+      return std::make_pair<BasicBlock *, BasicBlock *>(nullptr, nullptr);
----------------
Why just one? You could do:

auto *PredBB = IncomingBB;
while (auto *SinglePredBB = PredBB->getSinglePredecessor())
  PredBB = SinglePredBB;

at the top fo the function, and have a single set of logic to handle the whole thing?


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:189-192
+    if (!CI)
+      continue;
+    if (!CI->getType()->isIntegerTy(1))
+      continue;
----------------
Can simplify as:

  if (!CI || !CI->getType()->isIntegerTy(1))
    continue;


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:194
+
+    BP = (CI->getZExtValue() ? BranchProbability::getBranchProbability(
+                                   TrueWeight, TrueWeight + FalseWeight)
----------------
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.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:200
+    auto PredOutEdge = GetPredOutEdge(PN->getIncomingBlock(i), BB);
+    if (PredOutEdge.first == nullptr)
+      return;
----------------
No need to compare with nullptr, pointers are testable directly:

  if (PredOutEdge.first)


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:207
+    uint64_t PredTrueWeight, PredFalseWeight;
+    // 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
----------------
"we" -> "We"

I wonder, since what we have is fundamentally an upper bound, would it make sense to go ahead and apply this even when there is existing profile info?

Even for static hints, I can see this happening. For example, the pre-inlining code might have some relatively weak hint that is, after inlining, bounded by a powerful hint.

Or is your concern doing this without measuring the impact for PGO? If that's why, I would make that more clear -- we *intend* to always use this to refine weights, but PGO requires more careful tuning to make sure this doesn't regress anything.

Otherwise, I'd just go ahead and implement this as clamping any existing weights.


================
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))
----------------
"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"


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:213
+
+    // We can not infer anything useful when BP >= 50%.
+    if (BP >= BranchProbability(50, 100))
----------------
I would remind the reader that this is because all we have is an upper bound.


https://reviews.llvm.org/D36864