[PATCH] D26037: Add LoopSink pass for PGO.

[Hal Finkel via llvm-commits]

----- Original Message -----

> 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. 

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