<div dir="ltr">thanks. All done.<div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 18, 2017 at 2:50 PM, Chandler Carruth via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">chandlerc accepted this revision.<br>
chandlerc added a comment.<br>
This revision is now accepted and ready to land.<br>
<br>
Awesome. LGTM with the minor things below addressed. Let me know if any of this isn't make sense.<br>
<span class=""><br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>JumpThreading.cpp:194<br>
+<br>
+    BP = (CI->getZExtValue() ? BranchProbability::<wbr>getBranchProbability(<br>
+                                   TrueWeight, TrueWeight + FalseWeight)<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> chandlerc wrote:<br>
> > This code only handle i1 PHIs that directly feed conditional branches, right?<br>
> ><br>
> > 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.<br>
> There is an early check of one bit type. I am not clear what is the suggestion about (regarding extracting value).<br>
</span>Essentially...<br>
<br>
  assert(CI.getBitWidth() == 1 && "We only handle i1 constants here.");<br>
  BP = CI->isOne() ? ... : ...;<br>
<br>
You could also use `CI->isZero()`, but one seems more clear.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>JumpThreading.cpp:208-209<br>
</span><span class="">+    // FIXME: we currently only set the profile data when it is missing.<br>
+    // With PGO, this can be turned on too to refine the profile with<br>
</span><span class="">+    // context information.<br>
+    if (PredBr->extractProfMetadata(<wbr>PredTrueWeight, PredFalseWeight))<br>
----------------<br>
</span><span class="">chandlerc wrote:<br>
> "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"<br>
</span>This wording improvement for the comment would still be helpful I think.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>JumpThreading.cpp:163<br>
<span class="">+//  can not be greater than 1%.<br>
+<br>
</span>+static void updatePredecessorProfileMetada<wbr>ta(PHINode *PN, BasicBlock *BB) {<br>
----------------<br>
nit: spurious blank line...<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>JumpThreading.cpp:176<br>
+  // that leads to the PhiNode's incoming block:<br>
+  auto GetPredOutEdge = [](BasicBlock *IncomingBB, BasicBlock *PhiBB) -> std::pair<BasicBlock *, BasicBlock *> {<br>
+    auto *PredBB = IncomingBB;<br>
----------------<br>
nit: clang-format to fix line length<br>
<br>
<br>
<a href="https://reviews.llvm.org/D36864" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36864</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>