[PATCH] D21823: [Driver] Add flags for enabling both types of PGO Instrumentation

[Xinliang David Li via cfe-commits]

On Tue, Jul 19, 2016 at 5:06 PM, Vedant Kumar <vsk at apple.com> wrote:

>
> > On Jul 19, 2016, at 5:01 PM, Xinliang David Li <davidxl at google.com>
> wrote:
> >
> > The new behavior works really well. There is one follow up change I
> would like to discuss.
> >
> > The EQ form of the option -fprofile-generate= takes a directory name as
> the argument instead of the filename. Currently the compiler will create a
> default 'default.profraw' name for it, but this means, online merging won't
> be available unless environment variable is also used, which is not
> convenient.  I propose we change the default behavior to enable profile
> online merging (i.e., use default_%m.profraw as the default name) for the
> following reasons:
> >
> > 1) There is almost no downside enabling profiling merging by default --
> new capability is enabled with no lost functionality
>
> To be clear, this is a behavior change, but IMO the new behavior would be
> better.
>
Instead of creating programs that clobber over existing profiles by default
> they would merge into a set of profiles.
>
>
I consider this a bug fix -- as it makes a broken behavior into a working
one ;)

The only real behavior change is the default file name and number of
produced profile files may change -- but it is more likely the users of
-fprofile-generate=<> option expect to see the new behavior vs the old.

David



>
> > 2) WIth profile merging enabled for instrumented programs with
> instrumented shared libs, each lib will dump its own profile data in the
> same dir, but users of -fprofile-generate= option usually do not care about
> what/how many raw profile names are in the directory (note that GCC's
> behavior is one profile per object module), they just pack up the while
> directory. llvm-profdata also support merging (for indexed profile) with
> input-list-file option.
> > 3) GCC's has online merging support, so having online merging by default
> with this option matches up better the claim that it is a GCC compatible
> option.
> >
> > What do you think?
>
> + 1
>
> thanks,
> vedant
>
> >
> > thanks,
> >
> > David
> >
> >
> >
> > On Sat, Jul 9, 2016 at 4:39 PM, Sean Silva <chisophugis at gmail.com>
> wrote:
> > silvas added a comment.
> >
> > In http://reviews.llvm.org/D21823#479418, @davidxl wrote:
> >
> > > I should have brought it up earlier, but I forgot.    I think a better
> (and simpler) proposal is to make -fprofile-generate and -fprofile-use turn
> on IR based PGO.
> > >
> > > -fprofile-generate and -fprofile-use options were introduced by Diego
> (cc'ed) recently for GCC option compatibility. At that time, IR based PGO
> was not yet ready, so they were made aliases to FE instrumentation options
> -fprofile-instr-generate and -fprofile-instr-use.    Now I think it is time
> to make it right.   The documentation only says that these two options are
> gcc compatible options to control profile generation and use, but does not
> specify the underlying implementation. It is also likely that Google is the
> only user of this option. If a user using this option really want FE
> instrumentation, there is always -fprofile-instr-generate to use.  It also
> more likely that IR instrumentation is what user wants when using GCC
> compatible options (as they behave more similarly).
> >
> >
> > This SGTM.
> >
> > It may cause some user confusion since there is no obvious distinction
> between "profile generate" and "profile instr generate" from a user
> perspective. But we can avoid that with improved documentation.
> >
> > My main concern is that the existing documentation already says that
> -fprofile-generate behaves identically to -fprofile-instr-generate
> http://clang.llvm.org/docs/UsersManual.html#cmdoption-fprofile-generate
> > However, I think it is reasonable to change this behavior (and of course
> the documentation), as users of this option are likely using it for
> compatibility with GCC and so they likely don't care about the specifics of
> clang FEPGO.
> > We probably want to at least leave a small note in the documentation
> regarding this change of behavior.
> >
> >
> > Repository:
> >   rL LLVM
> >
> > http://reviews.llvm.org/D21823
> >
> >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160719/fa42379e/attachment.html>