[PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

On Mon, Jan 25, 2016 at 11:00 AM, Xinliang David Li <davidxl at google.com>
wrote:

> On Fri, Jan 22, 2016 at 1:43 PM, Sean Silva <chisophugis at gmail.com> wrote:
> > silvas added a comment.
> >
> > In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
> >
> >> For the longer term, one possible solution is to make FE based
> >>  instrumentation only used for coverage testing which can be turned on
> >>  with -fcoverage-mapping option (currently, -fcoverage-mapping can not
> >>  be used alone and must be used together with
> >>  -fprofile-instr-generate). To summarize:
> >>
> >> A. Current behavior:
> >>
> >>  -----------------------
> >>
> >> 1. -fprofile-instr-generate turns on FE based instrumentation
> >> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based
> instrumentation and coverage mapping data generation.
> >> 3. -fprofile-instr-use=<..> assumes profile data from FE
> instrumentation.
> >>
> >> B. Proposed new behavior:
> >>
> >>  --------------------------------
> >>
> >> 1. -fprofile-instr-generate turns on IR late instrumentation
> >> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
> >> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler
> warning
> >> 4. -fprofile-instr-use=<> will automatically determine how to use the
> profile data.
> >
> >
> > Very good observation that we can determine FE or IR automatically based
> on the input profdata. That simplifies things.
> >
> >> B.2) above can be done today for improved usability.
> >
> >
> > I don't see how this improves usability. In fact, it is confusing
> because it hijacks an existing option.
> >
> > Also B.3 causes existing user builds to emit a warning, which is
> annoying.
>
> Since it is pointless to specify -fcoverage-mapping option alone, why
> not do not automatically? Yes it means some behavior change (in a good
> way) and  some annoying warnings initially, but those are trivially
> fixable by the user.
>

"trivially fixable" needs to be weighed against the number of times it
needs to be fixed across the user base.

Anyway, emitting a warning for this is clearly a separate discussion
(separate even from the discussion of whether to accept just
`-fcoverage-mapping` without `-fprofile-instr-generate`), so let's not get
hung up on it.


>
>
> >
> > I would propose the following modification of B:
> >
> > C.:
> >
> > 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves
> exactly as before, except that it uses IR instrumentation)
> > 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend
> instrumentation and coverage. (i.e. behaves exactly as before)
>
> I am fine with this -- as long as the user does not need to bear the
> burden of understanding where and how the instrumentation is done.
>

Agreed.


>
> > 3. -fprofile-instr-use=<> automatically determines which method to use
> >
> > All existing user workflows continue to work, except for workflows that
> attempt to `llvm-profdata merge` some old frontend profile data (e.g. they
> have checked-in to version control and represents some special workload)
> with the profile data from new binaries.
> >
> > Concretely, imagine the following workflow:
> >
> >   clang -fprofile-instr-generate foo.c -o foo
> >   ./foo
> >   llvm-profdata merge default.profraw -o new.profdata
> >   llvm-profdata merge new.profdata
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata
> -o foo.profdata
> >   clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo
> >
> > I think this is a reasonable breakage. We would need to add a note in
> the release notes. Unfortunately this is not expected breakage if we claim
> to have forward compatibility for profdata (which IIRC Apple requires;
> @bogner?). But I think this case will be rare and exceptional enough that
> we can tolerate it:
> >
>
> The old profile data has to out live the program source versions which
> usually change fast. In reality, I don't expect profile data to live
> too long.
>

I don't either, but it's not unthinkable. E.g. a user may have the sources
of a dependency checked-in, and they only update the profdata for those
sources when they update the dependency.

But overall, I think that release notes mentioning the workaround for this
situation is sufficient.

-- Sean Silva


>
> > - a simple immediate workaround is to specify `-fcoverage-mapping`
> (which also adds some extra stuff, but works around the issue)
>
> This is a reasonable workaround
>
> > - Presumably
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata
> is regenerated with some frequency which is more frequent than upgrading
> the compiler, and so it is likely reasonable to regenerate it alongside a
> compiler upgrade, using the workaround until then.
> >
> >
> >
> >> B.1) needs a
> >
> >>  transition period before  the IR based instrumentation becomes
> >
> >>  stablized (and can be flipped to the default).  During the transition
> >
> >>  period, the behavior of 1) does not change, but a cc1 option can be
> >
> >>  used to turn on IR instrumentation (as proposed by Sean).
> >
> >
> > Just to clarify, users are not allowed to use cc1 options. The cc1
> option is purely for us as compiler developers to do integration and
> testing, put off some discussions for later, etc.
> >
>
> yes.
>
> thanks,
>
> David
>
> >
> > http://reviews.llvm.org/D15829
> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160125/cd33af5f/attachment-0001.html>