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

Tue May 24 17:21:05 PDT 2016

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/20160524/ddd1a67d/attachment.html>