llvm dev - Jun 2016 - The state of IRPGO (3 remaining work items)

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>

On Wed, May 25, 2016 at 9:22 AM, Xinliang David Li <davidxl at google.com>
wrote:
> 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.
>
One question for your David, related to merging indexed profiles. The
requirement that indexed profiles from different compiler versions can be
merged (with the intended result) implies that the CFG hash for a given
function must be the same (this is the change that actually breaks merging
IRPGO vs FEPGO). But the same issue in principle applies to IRPGO across
compiler versions. How likely do you think it will be that this hash may
change? I can think of two sources:
a) we decide to walk the CFG differently for computing the hash or decide
to use a different hash function etc.
b) pre-instrumentation passes change across compiler versions (e.g. inliner
makes different decisions in a newer compiler version)

`b)` definitely seems like it will be a problem. What do you think about a)?

-- Sean Silva

>
> 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/20160602/755527ce/attachment-0001.html>

On Thu, Jun 2, 2016 at 12:24 AM, Sean Silva <chisophugis at gmail.com>
wrote:
>
>
> On Wed, May 25, 2016 at 9:22 AM, Xinliang David Li <davidxl at
google.com>
> wrote:
>
>> 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.
>>
>
> One question for your David, related to merging indexed profiles. The
> requirement that indexed profiles from different compiler versions can be
> merged (with the intended result) implies that the CFG hash for a given
> function must be the same (this is the change that actually breaks merging
> IRPGO vs FEPGO). But the same issue in principle applies to IRPGO across
> compiler versions. How likely do you think it will be that this hash may
> change? I can think of two sources:
> a) we decide to walk the CFG differently for computing the hash or decide
> to use a different hash function etc.
> b) pre-instrumentation passes change across compiler versions (e.g.
> inliner makes different decisions in a newer compiler version)
>
> `b)` definitely seems like it will be a problem. What do you think about
> a)?
>
Strictly speaking, the requirement is just that index format change is
backward compatible -- this includes changes of PGO name encoding scheme.
For content  function hashing, unless we systematically change the hash
algorithm which will require a version bump, local compiler changes (such
as bug fixes) that lead to hashing to be different for individual functions
should not be considered breaking compatibility. This can happen for both
IR and FE instrumentation.

The above should apply to b) as well. It is true that b) is more sensitive
to compiler version changes in a way that more individual functions'
profile may get invalidated due to version change, but I think this is by
design -- keeping profile fully compatible across compiler versions is not
the major intended use cases.  However if cfg traverse order changes, I
think a version bump is needed. This however should be fairly rarely.

David


> -- Sean Silva
>
>
>>
>> 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/20160602/18ed2d45/attachment.html>