[PATCH] D25963: [LoopUnroll] Implement profile-based loop peeling

[Xinliang David Li via llvm-commits]

On Tue, Nov 15, 2016 at 4:53 PM, Michael Kuperstein <mkuper at google.com>
wrote:

> mkuper added a subscriber: bogner.
> mkuper added a comment.
>
> In https://reviews.llvm.org/D25963#596720, @anemet wrote:
>
> > In https://reviews.llvm.org/D25963#596582, @mkuper wrote:
> >
> > > Thanks, Adam!
> > >  I'll fix the cosmetics, but I'm still not sure what to do about the
> branch weight adjustments.
> >
> >
> > Wasn't @davidxl working on fixing them?
>
>
> Not that I know of. @davidxl, did I miss something?
>
> Also, phab seems to have swallowed a long comment I wrote about that. :-\
>
> Basically, the underlying issue is that clang doesn't actually record the
> branch weights it gets from the profile. Rather, it records the branch
> weights it thinks are better for the purpose of branch probability
> estimation.  From CodeGenPGO.cpp:
> /// According to Laplace's Rule of Succession, it is better to compute the
> /// weight based on the count plus 1, so universally add 1 to the value.
> While this may be good from a branch-probability standpoint, it throws off
> using the weights for trip count estimates, when the exit weight is small.
>
> I see 3 options:
>
> 1. Keep what I have here as a band-aid.
>

Probably not.  it makes IR based PGO wrong, and also defeats the purpose of
the original adjustment in the first place. In other words, 1) is strictly
worse than 3) below.


> 2. Don't do the -1 adjustment here - hopefully, in reality, we don't care
> about the cases where it makes a difference. I'll need to benchmark this,
> though.
>


See my reply. We should go for 2.


> 3. Remove the +1 adjustment in clang, and instead do it where we actually
> use the branch weights to estimate probabilities, that is, in BPI. Of
> course, there may be other places that read the weights but really want the
> adjusted weights (and, say, assume weights are never 0). I asked @bogner
> about this on IRC, and he seemed ambivalent.
>

This has been discussed many times in the past.  The conclusion is that we
will keep FE profile handling as it is today.

David



>
> What do you think?
>
>
> https://reviews.llvm.org/D25963
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161115/71d80cb4/attachment.html>