On Tue, Jul 19, 2016 at 5:01 PM, Xinliang David Li <davi...@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 <chisophu...@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 >> >> >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
