On Tue, Jul 19, 2016 at 5:06 PM, Vedant Kumar <v...@apple.com> wrote:
> > > On 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 > > 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 <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
