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

[Xinliang David Li via llvm-dev]

On Wed, May 25, 2016 at 12:12 PM, Sean Silva <chisophugis at gmail.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.
>>
>>
>> - 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.
>>
>> 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 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.
>>
>>
>> - 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/D15828 did 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).
>>
>>
>> - 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).
>>
>
>
> Rong, I was just looking at implementing a fix for this, but noticed
> something. Can we get rid of the "InLTO" argument to getPGOFuncName if we
> unconditionally apply the funcname metadata to all functions?
>

Are you proposing always emitting the meta data for internal functions?

David



>
> -- Sean Silva
>
>
>>
>> 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.
>>
>> 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.
>>
>> 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.
>>
>> 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.
>>
>> -- Sean Silva
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160525/965f9377/attachment.html>