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

[Michael Kuperstein via llvm-commits]

SGTM.
I'll remove the adjustment.

On Tue, Nov 15, 2016 at 4:57 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> 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/f90895c3/attachment.html>