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

[Xinliang David Li via llvm-commits]

thanks. All done.

David

On Fri, Aug 18, 2017 at 2:50 PM, Chandler Carruth via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170818/bc8ca1c8/attachment.html>