<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 19, 2016 at 5:06 PM, Vedant Kumar <span dir="ltr"><<a href="mailto:vsk@apple.com" target="_blank">vsk@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On Jul 19, 2016, at 5:01 PM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> wrote:<br>
><br>
> The new behavior works really well. There is one follow up change I would like to discuss.<br>
><br>
> 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:<br>
><br>
> 1) There is almost no downside enabling profiling merging by default -- new capability is enabled with no lost functionality<br>
<br>
</span>To be clear, this is a behavior change, but IMO the new behavior would be<br>
better. <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Instead of creating programs that clobber over existing profiles by default<br>
they would merge into a set of profiles.<br>
<span class=""><br></span></blockquote><div><br></div><div>I consider this a bug fix -- as it makes a broken behavior into a working one ;)</div><div><br></div><div>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.</div><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
> 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.<br>
> 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.<br>
><br>
> What do you think?<br>
<br>
</span>+ 1<br>
<br>
thanks,<br>
vedant<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> thanks,<br>
><br>
> David<br>
><br>
><br>
><br>
> On Sat, Jul 9, 2016 at 4:39 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
> silvas added a comment.<br>
><br>
> In <a href="http://reviews.llvm.org/D21823#479418" rel="noreferrer" target="_blank">http://reviews.llvm.org/D21823#479418</a>, @davidxl wrote:<br>
><br>
> > 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.<br>
> ><br>
> > -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).<br>
><br>
><br>
> This SGTM.<br>
><br>
> 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.<br>
><br>
> My main concern is that the existing documentation already says that -fprofile-generate behaves identically to -fprofile-instr-generate <a href="http://clang.llvm.org/docs/UsersManual.html#cmdoption-fprofile-generate" rel="noreferrer" target="_blank">http://clang.llvm.org/docs/UsersManual.html#cmdoption-fprofile-generate</a><br>
> 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.<br>
> We probably want to at least leave a small note in the documentation regarding this change of behavior.<br>
><br>
><br>
> Repository:<br>
>   rL LLVM<br>
><br>
> <a href="http://reviews.llvm.org/D21823" rel="noreferrer" target="_blank">http://reviews.llvm.org/D21823</a><br>
><br>
><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>