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

[Sean Silva via llvm-dev]

On Mon, 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.
>
>>
>>
>> - 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.
>>
>>
> 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.
>

Yes, since -fprofile-instr-use can automatically detect, the only issue is
merging with old profiles. The simplest solution is to add a driver flag
-fuse-the-old-pgo-because-i-want-to-merge-with-old-profiles (name is a
bikeshed) which temporarily allows users to continue to use FE PGO. I don't
see a reason why we would want FEPGO in the long term; we can always use
IRPGO with no pre-instrumentation passes to get the same effect.


>
>
>>
>> - 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).
>>
>>
>>
> Yes, Rong is re-collecting performance data before submitting the patch.
>

Great!


>
>
>> - 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?
>

Sure: https://llvm.org/bugs/show_bug.cgi?id=27867


>
>
>>
>> 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.
>

Great!


>
>
>
>> 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.
>

That might work, but would be tricky since there might be linkonce_odr
functions also in the translation unit and so not all functions would be
missing profile counters.

-- Sean Silva


>
> thanks,
>
> David
>
>
>>
>> -- Sean Silva
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160524/4ff4c13f/attachment.html>