[PATCH] D26037: Add LoopSink pass for PGO.

[Xinliang David Li via llvm-commits]

On Fri, Oct 28, 2016 at 10:39 AM, Hal Finkel <hfinkel at anl.gov> wrote:

>
> ------------------------------
>
> *From: *"Xinliang David Li" <davidxl at google.com>
> *To: *"Hal Finkel" <hfinkel at anl.gov>
> *Cc: *"陈德颢" <danielcdh at gmail.com>, "Junbum Lim" <junbuml at codeaurora.org>,
> "llvm-commits" <llvm-commits at lists.llvm.org>, anna at azul.com,
> reviews+D26037+public+89d2f1349c56fe36 at reviews.llvm.org
> *Sent: *Friday, October 28, 2016 12:26:39 PM
> *Subject: *Re: [PATCH] D26037: Add LoopSink pass for PGO.
>
>
>
> On Fri, Oct 28, 2016 at 10:20 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>
>>
>> ------------------------------
>>
>> *From: *"Xinliang David Li" <davidxl at google.com>
>> *To: *reviews+D26037+public+89d2f1349c56fe36 at reviews.llvm.org
>> *Cc: *"陈德颢" <danielcdh at gmail.com>, "Hal Finkel" <hfinkel at anl.gov>,
>> "Junbum Lim" <junbuml at codeaurora.org>, "llvm-commits" <
>> llvm-commits at lists.llvm.org>, anna at azul.com
>> *Sent: *Friday, October 28, 2016 12:11:04 PM
>> *Subject: *Re: [PATCH] D26037: Add LoopSink pass for PGO.
>>
>>
>>
>> On Fri, Oct 28, 2016 at 9:40 AM, Dehao Chen <danielcdh at gmail.com> wrote:
>>
>>> danielcdh added a comment.
>>>
>>> We need this pass to remove LCSSA nodes because this will be the last
>>> loop pass.
>>>
>>> Another question is: shall we pin this pass to PGO only? If not, we can
>>> simply add it to lib/Transforms/IPO/PassManagerBuilder.cpp
>>>
>>
>> Static branch prediction is conservative in predicting loop iterations
>> (loop scale). This means it is likely some blocks (if wrapped under
>> __builtin_expect) inside the loop will be wrongly marked as colder than
>> preheader  which can trigger loop sinking wrongly.
>>
>> Can you please elaborate? If we statically predict a modest number of
>> iterations, which seems like a reasonable trade off, and we have a block
>> which is expected to not execute (because of __builtin_expect), then is it
>> obviously wrong that it should be colder than the preheader? If the
>> opposite of __builtin_expect is "almost never",
>>
>
>
> My observation is that the interpretation and use  of '__builtin_expect'
> varies a lot from user to user. Some consider 80% or even as low as ~70%
> probability as expected, while some considers >99%. Even with 99% expected
> probability, it still can get wrong. For instance in a loop with 1 million
> iterations, the cold block will execute 10k times -- still much hotter than
> the preheader.
>
> Indeed; __builtin_expect would be better with a percentage ;)
>
> However, I'm not sure that this is the best approach. Just because you
> have profiling data, doesn't mean you have complete profiling data. I think
> that it would be better if we indicated the veracity/confidence of the
> profiling data in the profiling metadata, and then used that.
>
>
Good point -- so checking option is not the best approach here.  Better
just make this into standard pipeline and let the pass to determine what to
do depending on the actual availability of PGO data.   We assume PGO users
will use representative training data (otherwise he will shoot himself in
the foot) so profile metadata from PGO has high confidence. Note that PGO
meta data will overwrite meta data from user annotation, so there is no
need to mark meta data with additional profile accuracy info.

David




>  -Hal
>
>
> David
>
>
>
>> which is true for error paths and the like, then that seems not
>> unreasonable. Why would loop sinking hurt there?
>>
>>
>
>
>
>
>> Thanks again,
>> Hal
>>
>>   Making this pass depending on PGO is a safer thing to do.
>>
>> David
>>
>>
>>>
>>> https://reviews.llvm.org/D26037
>>>
>>>
>>>
>>>
>>
>>
>>
>> --
>> Hal Finkel
>> Lead, Compiler Technology and Programming Languages
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>
>
>
>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161028/ad3e5a05/attachment.html>