<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 25, 2016 at 11:00 AM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.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="">On Fri, Jan 22, 2016 at 1:43 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/D15829#333902" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15829#333902</a>, @davidxl wrote:<br>
><br>
>> For the longer term, one possible solution is to make FE based<br>
>> instrumentation only used for coverage testing which can be turned on<br>
>> with -fcoverage-mapping option (currently, -fcoverage-mapping can not<br>
>> be used alone and must be used together with<br>
>> -fprofile-instr-generate). To summarize:<br>
>><br>
>> A. Current behavior:<br>
>><br>
>> -----------------------<br>
>><br>
>> 1. -fprofile-instr-generate turns on FE based instrumentation<br>
>> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based instrumentation and coverage mapping data generation.<br>
>> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.<br>
>><br>
>> B. Proposed new behavior:<br>
>><br>
>> --------------------------------<br>
>><br>
>> 1. -fprofile-instr-generate turns on IR late instrumentation<br>
>> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping<br>
>> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning<br>
>> 4. -fprofile-instr-use=<> will automatically determine how to use the profile data.<br>
><br>
><br>
> Very good observation that we can determine FE or IR automatically based on the input profdata. That simplifies things.<br>
><br>
>> B.2) above can be done today for improved usability.<br>
><br>
><br>
> I don't see how this improves usability. In fact, it is confusing because it hijacks an existing option.<br>
><br>
> Also B.3 causes existing user builds to emit a warning, which is annoying.<br>
<br>
</span>Since it is pointless to specify -fcoverage-mapping option alone, why<br>
not do not automatically? Yes it means some behavior change (in a good<br>
way) and some annoying warnings initially, but those are trivially<br>
fixable by the user.<br></blockquote><div><br></div><div>"trivially fixable" needs to be weighed against the number of times it needs to be fixed across the user base.</div><div><br></div><div>Anyway, emitting a warning for this is clearly a separate discussion (separate even from the discussion of whether to accept just `-fcoverage-mapping` without `-fprofile-instr-generate`), so let's not get hung up on it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
<br>
><br>
> I would propose the following modification of B:<br>
><br>
> C.:<br>
><br>
> 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves exactly as before, except that it uses IR instrumentation)<br>
> 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend instrumentation and coverage. (i.e. behaves exactly as before)<br>
<br>
</span>I am fine with this -- as long as the user does not need to bear the<br>
burden of understanding where and how the instrumentation is done.<br></blockquote><div><br></div><div>Agreed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> 3. -fprofile-instr-use=<> automatically determines which method to use<br>
><br>
> All existing user workflows continue to work, except for workflows that attempt to `llvm-profdata merge` some old frontend profile data (e.g. they have checked-in to version control and represents some special workload) with the profile data from new binaries.<br>
><br>
> Concretely, imagine the following workflow:<br>
><br>
> clang -fprofile-instr-generate foo.c -o foo<br>
> ./foo<br>
> llvm-profdata merge default.profraw -o new.profdata<br>
> llvm-profdata merge new.profdata /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata -o foo.profdata<br>
> clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo<br>
><br>
> I think this is a reasonable breakage. We would need to add a note in the release notes. Unfortunately this is not expected breakage if we claim to have forward compatibility for profdata (which IIRC Apple requires; @bogner?). But I think this case will be rare and exceptional enough that we can tolerate it:<br>
><br>
<br>
</span>The old profile data has to out live the program source versions which<br>
usually change fast. In reality, I don't expect profile data to live<br>
too long.<br></blockquote><div><br></div><div>I don't either, but it's not unthinkable. E.g. a user may have the sources of a dependency checked-in, and they only update the profdata for those sources when they update the dependency.</div><div><br></div><div>But overall, I think that release notes mentioning the workaround for this situation is sufficient.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> - a simple immediate workaround is to specify `-fcoverage-mapping` (which also adds some extra stuff, but works around the issue)<br>
<br>
</span>This is a reasonable workaround<br>
<span class=""><br>
> - Presumably /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata is regenerated with some frequency which is more frequent than upgrading the compiler, and so it is likely reasonable to regenerate it alongside a compiler upgrade, using the workaround until then.<br>
><br>
><br>
><br>
>> B.1) needs a<br>
><br>
>> transition period before the IR based instrumentation becomes<br>
><br>
>> stablized (and can be flipped to the default). During the transition<br>
><br>
>> period, the behavior of 1) does not change, but a cc1 option can be<br>
><br>
>> used to turn on IR instrumentation (as proposed by Sean).<br>
><br>
><br>
> Just to clarify, users are not allowed to use cc1 options. The cc1 option is purely for us as compiler developers to do integration and testing, put off some discussions for later, etc.<br>
><br>
<br>
</span>yes.<br>
<br>
thanks,<br>
<br>
David<br>
<br>
><br>
> <a href="http://reviews.llvm.org/D15829" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15829</a><br>
><br>
><br>
><br>
</blockquote></div><br></div></div>