[PATCH] D36333: Move the SampleProfileLoader right after EarlyFPM.

[Xinliang David Li via llvm-commits]

There is no suggestion of redesign right now, but just more comments and
documentation.

On Fri, Aug 4, 2017 at 1:50 PM, Dehao Chen via Phabricator <
reviews at reviews.llvm.org> wrote:

> danielcdh added inline comments.
>
>
> ================
> Comment at: lib/Passes/PassBuilder.cpp:552-553
> +  if (PGOOpt && !PGOOpt->SampleProfileFile.empty()) {
> +    // Annotate sample profile right after early FPM to ensure freshness
> of
> +    // the debug info.
> +    MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile));
> ----------------
> davidxl wrote:
> > chandlerc wrote:
> > > Above, you say that this is because SampleProfileLoaderPass needs to
> be here because it inlines calls. Here you talk about debug info
> freshness... Which is it?
> > >
> > > The debug info freshness makes plenty of sense to me. If
> SampleProfileLoaderPass is *doing inlining*, I find that deeply surprising
> and confusing. I'm not suggesting that we need to change the design of this
> right now, but I think the comment needs to be *really* clear because lots
> of other people will be surprised by this and I'd suggest a FIXME to
> revisit how this is structured. (Regardless of whether inlining makes sense
> here, I think it should *definitely* be separated from what appears to be a
> pass that merely annotates the IR from a profile.)
> > Should probably add a reference to the autofdo CGO 2016 paper which has
> detailed description of how autofdo's context senstive profile data is
> organized and the profile driven inline and branch profile annotation
> design.
> Yes, we do invoke InlineFunction directly in SampleProfileLoader. There is
> a great many details about how we end up with this design in out cgo2016
> paper. And I could explain to you in person if you like.
>
> The fundamental requirement is that we need to make the IR similar with
> the binary that is used for profiling, in order to annotate sample profile
> accurately.
>
> To get to that goal, we have choose to first inline inside the
> SampleProfileLoader before annotation. During the inlining we need to make
> the inline decision by interacting with SampleProfile interface. By doing
> this inside the SampleProfileLoader, we lock the use of SampleProfile
> interface within this file, instead of making it escape into the inliner.
>
> Alternatively, we could have a separate inliner pass to do this job. But
> that inliner pass need to read the SampleProfile too. So if we do that, we
> need to read the sample profile twice.
>
> I'm open to discuss about other design alternatives too.
>
>
> https://reviews.llvm.org/D36333
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170804/35ebef9b/attachment.html>