[llvm-dev] The state of IRPGO (3 remaining work items)

Wed May 25 09:22:07 PDT 2016

It sounds to me we are likely to converge on the following:

1) Making IR/llvm based PGO the default;
2) Enhance -fcoverage-mapping such that it automatically turns on FE based
instrumentation
3) if -fcoverage-mapping is used together with -fprofile-instr-generate,
-fcoverage-mapping serves as a switch to turn on FE based instrumetnation

All the above are transparent to users.

The following are for advanced usage:

4) have a new option to explicitly switch instrumentation flavor to be FE
based
5) have a new option to turn off part of pre-instrumentation
cleanup/simplification passes for users who want very stable profile for
stable library sources *

* 4 and 5 serves the same purpose so 5 may not be necessary.

thanks,

On Tue, May 24, 2016 at 5:21 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Tue, May 24, 2016 at 3:41 PM, Vedant Kumar <vsk at apple.com> wrote:
>
>>
>> > On May 23, 2016, at 8:56 PM, Xinliang David Li <davidxl at google.com>
>> wrote:
>> >
>> > On Mon, May 23, 2016 at 8:23 PM, Sean Silva <chisophugis at gmail.com>
>> wrote:
>> > Jake and I have been integrating IRPGO on PS4, and we've identified 3
>> remaining work items.
>> >
>> > Sean, thanks for the write up. It matches very well with what we think
>> as well.
>>
>> + 1
>>
>>
>> > - Driver changes
>> >
>> > We'd like to make IRPGO the default on PS4. We also think that it would
>> be beneficial to make IRPGO the default PGO on all platforms (coverage
>> would continue to use FE instr as it does currently, of course). In
>> previous conversations (e.g. http://reviews.llvm.org/D15829) it has come
>> up that Apple have requirements that would prevent them from moving to
>> IRPGO as the default PGO, at least without a deprecation period of one or
>> two releases.
>>
>> Sean pointed out the problematic scenario in D15829 (in plan "C"):
>>
>> ```
>> 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.
>> ```
>>
>> We can address this issue by (1) making sure llvm-profdata emits a
>> helpful warning when merging an FE-based profile with an IR-based one, and
>> (2) keeping an option to use FE instrumentation for PGO. Having (2) helps
>> people who can't (or don't want) to switch to IR PGO.
>>
>>
>> > I'd like to get consensus on a path forward.
>> > As a point of discussion, how about we make IRPGO the default on all
>> platforms except Apple platforms.
>>
>> I'd really rather not introduce this inconsistency. I'm worried that it
>> might lead to Darwin becoming a second-tier platform for PGO.
>>
>> Fred (CC'd) is following up with some of our internal users to check if
>> we can change the default behavior of -fprofile-instr-generate. He should
>> be able to chime in on this soon.
>>
>
> Great!
>
>
>>
>> At its core I don't think -fprofile-instr-generate *implies* FE-based
>> instrumentation. So, I'd like to see the driver do this (on all platforms):
>>
>>   * -fprofile-instr-generate: IR instrumentation
>>   * -fprofile-instr-generate=IR: IR instrumentation
>>   * -fprofile-instr-generate=FE: FE instrumentation
>>   * -fprofile-instr-generate -fcoverage-mapping: FE + coverage
>> instrumentation
>>
>> It's a bit ugly because the meaning of -fprofile-instr-generate becomes
>> context-sensitive. But, (1) it doesn't break existing common workflows and
>> (2) it makes it easier to ship IRPGO. The big caveat here is that we'll
>> need to wait a bit and see if our internal users are OK with this.
>>
>
> Is there a reason to even have the possibility for FEPGO in the long run?
> From what I can tell, at most we would add a -fuse-the-old-pgo-because-i-
> want-to-merge-with-old-profiles option to hold people over until they can
> regenerate their profiles with the current compiler. We can add a flag to
> control what pre-instrumentation is done to retain the source-level
> robustness of FEPGO (e.g. -fpgo-no-simplify-before-instrumenting or
> something).
>
>
>> One alternative is to introduce a separate driver flag for IRPGO. This
>> might not work well for Sony's existing users. I'd be interested in any
>> feedback about this approach.
>>
>
> Personally, I would prefer to maintaining command line compatibility for
> PGO in Clang (i.e. users don't have to modify their build systems).
>
>
> -- Sean Silva
>
>
>>
>>
>> > I really don't like fragmenting things like this (e.g. if a third-party
>> tests "clang's" PGO they will get something different depending on the
>> platform), but I don't see another way given Apple's constraints.
>> >
>> > I'd like to see IRPGO to be the default as well, but the first thing we
>> need is a driver level option to make the switch (prof-gen) -- currently we
>> rely on -Xclang option to switch between two modes, which is less than
>> ideal.
>> >
>> > If the concern from Apple is that the old profile still need to work,
>> then this is problem already solved. The reason is that -fprofile-instr-use
>> can automatically detect the type of the profile and switch the mode.
>>
>> It's not just that. As Sean pointed out, we're concerned about old
>> profiles inter-operating poorly with new ones.
>>
>> thanks,
>> vedant
>>
>>
>> > - Pre-instrumentation passes
>> >
>> > Pre-instrumentation optimization has been critical for reducing the
>> overhead of PGO for the PS4 games we tested (as expected). However, in our
>> measurements (and we are glad to provide more info) the main benefit was
>> inlining (also as expected). A simple pass of inlining at threshold 100
>> appeared to give all the benefits. Even inlining at threshold 0 gave almost
>> all the benefits. For example, the passes initially proposed in
>> http://reviews.llvm.org/D15828did not improve over just inlining with
>> threshold 100.
>> >
>> > (due to PR27299 we also need to add simplifycfg after inlining to clean
>> up, but this doesn't affect the instrumentation overhead in our
>> measurements)
>> >
>> > Bottom line: for our use cases, inlining does all the work, but we're
>> not opposed to having more passes, which might be beneficial for non-game
>> workloads (which is most code).
>> >
>> >
>> >
>> > Yes, Rong is re-collecting performance data before submitting the patch.
>> >
>> > - Warnings
>> >
>> > We identified 3 classes of issues which manifest as spammy warnings
>> when applying profile data with IRPGO (these affect FEPGO also I believe,
>> but we looked in depth at IRPGO):
>> >
>> > 1. The main concerning one is that getPGOFuncName mangles the filename
>> into the counter name. This causes us to get
>> instrprof_error::unknown_function when the pgo-use build is done in a
>> different build directory from the training build (which is a reasonable
>> thing to support). In this situation, PGO data is useless for all `static`
>> functions (and as a byproduct results in a huge volume of warnings).
>> >
>> > This can be enhanced with an user option to override the behavior. Can
>> you help filing a tracking bug?
>> >
>> >
>> > 2. In different TU's, pre-instr inlining might make different inlining
>> decisions (for example, different functions may be available for inlining),
>> causing hash mismatch errors (instrprof_error::hash_mismatch). In building
>> a large game, we only saw 8 instance of this, so it is not as severe as 1,
>> but would be good to fix.
>> >
>> >
>> > Rong has a patch addressing that -- will submit after cleanup pass
>> change is done.
>> >
>> >
>> > 3. A .cpp file may be compiled and put into an archive, but then not
>> selected by the linker and will therefore not result in a counter in the
>> profraw. When compiling this file with pgo-use,
>> instrprof_error::unknown_function will result and a warning will be emitted.
>> >
>> > yes -- this is a common problem to other compilers as well.
>> >
>> >
>> >
>> > Case 1 can be fixed using a function hash or other unique identifier
>> instead of a file path. David, in D20195 you mentioned that Rong was
>> working on a patch that would fix 2; we are looking forward to that.
>> >
>> >
>> > Right.
>> >
>> > For 3, I unfortunately do not know of any solution. I don't think there
>> is a way for us to make this warning reliable in the face of this
>> circumstance. So my conclusion is that instrprof_error::unknown_function at
>> least must be defaulted to off unfortunately.
>> >
>> > yes, this can be annoying. If the warnings can be buffered, then the
>> compiler can check if this is due to missing profile for the whole file and
>> can reduce the warnings into one single warning (source file has no profile
>> data).  Making it off by default sounds fine to me too if it is too noisy.
>> >
>> > thanks,
>> >
>> > David
>> >
>> >
>> > -- Sean Silva
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160525/16ab93d1/attachment.html>