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

[Sean Silva via cfe-commits]

On Tue, 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
> 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?
>
> thanks,
>
> David
>

This SGTM.

May only slight concern is making sure that tests which use globbing (which
`REQUIRES: shell`) to deal with %m expansions are kept separate if possible
-- otherwise windows (or windows-hosted, e.g. PS4) test coverage suffers.
Most of our tests use `env LLVM_PROFILE_FILE=` anyway so this hopefully
doesn't affect too many tests.

-- Sean Silva


>
>
>
> 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/6bda720d/attachment.html>