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

> On Jun 2, 2016, at 5:30 PM, Sean Silva <chisophugis at gmail.com>
wrote:
>  
> This also means that if the consensus is that -fprofile-instr-generate
should really change its meaning to mean IRPGO, I’m open to having this internal
patch on our side.
> 
> Yeah, it sounds like someone is going to have to keep a "private
patch" to change the default. At that point doing a switch on the triple in
upstream seems preferable though :/
I don’t see why a private patch would be needed.
> So I propose the following (which is equivalent to what you proposed, but
starting to put specific option names):
> 1. Add -fprofile-instr-generate=stable and
-fprofile-instr-generate=unstable
That sounds like the right approach. I don’t have a strong opinion about the
=stable/unstable names.
> a) Indexed profiles for -fprofile-instr-generate=stable are guaranteed to
be compatible across compiler releases (which will include existing releases).
Additionally, -fprofile-instr-generate=stable can be used in combination with
-fcoverage-mapping.
> b) Indexed profiles for -fprofile-instr-generate=unstable are not
guaranteed to be compatible across compiler releases. It is an error to use this
in conjunction with -fcoverage-mapping
> c) -fprofile-instr-generate defaults to one of the two in a way that meets
vendor needs, via private patches or an upstream switch on the triple or
whatever. It can eventually be deprecated or whatever.
Typically we handle things like this by staging the change over a release or
two. If we add those two new options (=stable, =unstable), we can have one
release where the default matches the old behavior, and then change the default
for the following release. Someone relying on the current behavior would then be
able to change their builds to use the =stable option so that they won’t be
broken when the default switches.
> 
> -fprofile-instr-generate=stable would basically correspond to FEPGO / FE
coverage instrumentation. -fprofile-instr-generate=unstable would basically
correspond to IRPGO.
> 
> What do you think?
> 
> -- Sean Silva
>  
> 
> I also think a new option is cleaner (again I don’t care about the name),
but if people feel that -fprofile-instr-generate=[llvm|clang] is easier, then
this works for me too.
> 
> Fred
> 
>> 
>> -- Sean Silva
>>  
>> 
>> Thanks!
>> Fred
>> 
>>> -- Sean Silva
>>> 
>>>  
>>> 
>>> Fred
>>> 
>>>>  
>>>> 
>>>> 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 <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/9fb72f9c/attachment-0001.html>

On Thu, Jun 2, 2016 at 5:45 PM, Bob Wilson <bob.wilson at apple.com>
wrote:
>
> On Jun 2, 2016, at 5:30 PM, Sean Silva <chisophugis at gmail.com>
wrote:
>
>
>> This also means that if the consensus is that -fprofile-instr-generate
>> should really change its meaning to mean IRPGO, I’m open to having this
>> internal patch on our side.
>>
>
> Yeah, it sounds like someone is going to have to keep a "private
patch" to
> change the default. At that point doing a switch on the triple in upstream
> seems preferable though :/
>
>
> I don’t see why a private patch would be needed.
>
On PS4 IRPGO is the default (and actually only; release-to-release
stability of profiles is not an issue, the benefit of IRPGO is too great,
and the support cost of multiple PGO's is undesirable given the overhead
issues with FEPGO. Additionally, we don't plan to invest development effort
in FEPGO as a peak performance configuration).

>
> So I propose the following (which is equivalent to what you proposed, but
> starting to put specific option names):
> 1. Add -fprofile-instr-generate=stable and
> -fprofile-instr-generate=unstable
>
>
> That sounds like the right approach.
>
Great. I'll post a patch in the next couple days.

> I don’t have a strong opinion about the =stable/unstable names.
>
> a) Indexed profiles for -fprofile-instr-generate=stable are guaranteed to
> be compatible across compiler releases (which will include existing
> releases). Additionally, -fprofile-instr-generate=stable can be used in
> combination with -fcoverage-mapping.
> b) Indexed profiles for -fprofile-instr-generate=unstable are not
> guaranteed to be compatible across compiler releases. It is an error to use
> this in conjunction with -fcoverage-mapping
> c) -fprofile-instr-generate defaults to one of the two in a way that meets
> vendor needs, via private patches or an upstream switch on the triple or
> whatever. It can eventually be deprecated or whatever.
>
>
> Typically we handle things like this by staging the change over a release
> or two. If we add those two new options (=stable, =unstable), we can have
> one release where the default matches the old behavior, and then change the
> default for the following release. Someone relying on the current behavior
> would then be able to change their builds to use the =stable option so that
> they won’t be broken when the default switches.
>
Good to know what processes you have in place.

-- Sean Silva

>
>
> -fprofile-instr-generate=stable would basically correspond to FEPGO / FE
> coverage instrumentation. -fprofile-instr-generate=unstable would basically
> correspond to IRPGO.
>
> What do you think?
>
> -- Sean Silva
>
>
>>
>> I also think a new option is cleaner (again I don’t care about the
name),
>> but if people feel that -fprofile-instr-generate=[llvm|clang] is
easier,
>> then this works for me too.
>>
>> Fred
>>
>>
>> -- Sean Silva
>>
>>
>>>
>>> Thanks!
>>> Fred
>>>
>>> -- Sean Silva
>>>
>>>
>>>
>>>>
>>>> Fred
>>>>
>>>>
>>>>
>>>>>
>>>>> 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/452ea34e/attachment.html>

> On Jun 2, 2016, at 6:07 PM, Sean Silva <chisophugis at gmail.com>
wrote:
> 
> 
> 
> On Thu, Jun 2, 2016 at 5:45 PM, Bob Wilson <bob.wilson at apple.com
<mailto:bob.wilson at apple.com>> wrote:
> 
>> On Jun 2, 2016, at 5:30 PM, Sean Silva <chisophugis at gmail.com
<mailto:chisophugis at gmail.com>> wrote:
>>  
>> This also means that if the consensus is that -fprofile-instr-generate
should really change its meaning to mean IRPGO, I’m open to having this internal
patch on our side.
>> 
>> Yeah, it sounds like someone is going to have to keep a "private
patch" to change the default. At that point doing a switch on the triple in
upstream seems preferable though :/
> 
> I don’t see why a private patch would be needed.
> 
> On PS4 IRPGO is the default (and actually only; release-to-release
stability of profiles is not an issue, the benefit of IRPGO is too great, and
the support cost of multiple PGO's is undesirable given the overhead issues
with FEPGO. Additionally, we don't plan to invest development effort in
FEPGO as a peak performance configuration).
Well, it’s up to you, but you could just as well tell everyone doing PS4
development to use the new -fprofile-instr-generate=unstable (or whatever it
ends up being called). If “FEPGO” doesn’t work for you, then presumably no one
is using the existing -fprofile-instr-generate option for PS4….
>  
> 
>> So I propose the following (which is equivalent to what you proposed,
but starting to put specific option names):
>> 1. Add -fprofile-instr-generate=stable and
-fprofile-instr-generate=unstable
> 
> That sounds like the right approach.
> 
> Great. I'll post a patch in the next couple days.
>  
> I don’t have a strong opinion about the =stable/unstable names.
> 
>> a) Indexed profiles for -fprofile-instr-generate=stable are guaranteed
to be compatible across compiler releases (which will include existing
releases). Additionally, -fprofile-instr-generate=stable can be used in
combination with -fcoverage-mapping.
>> b) Indexed profiles for -fprofile-instr-generate=unstable are not
guaranteed to be compatible across compiler releases. It is an error to use this
in conjunction with -fcoverage-mapping
>> c) -fprofile-instr-generate defaults to one of the two in a way that
meets vendor needs, via private patches or an upstream switch on the triple or
whatever. It can eventually be deprecated or whatever.
> 
> Typically we handle things like this by staging the change over a release
or two. If we add those two new options (=stable, =unstable), we can have one
release where the default matches the old behavior, and then change the default
for the following release. Someone relying on the current behavior would then be
able to change their builds to use the =stable option so that they won’t be
broken when the default switches.
> 
> Good to know what processes you have in place.
The “we” in my reply was referring to the LLVM project. It’s not about what
processes either Apple or I might use. We, the LLVM community, have a
responsibility to provide a good migration path for our users when we decide to
change the behavior of an existing feature.
> 
> -- Sean Silva
>  
> 
>> 
>> -fprofile-instr-generate=stable would basically correspond to FEPGO /
FE coverage instrumentation. -fprofile-instr-generate=unstable would basically
correspond to IRPGO.
>> 
>> What do you think?
>> 
>> -- Sean Silva
>>  
>> 
>> I also think a new option is cleaner (again I don’t care about the
name), but if people feel that -fprofile-instr-generate=[llvm|clang] is easier,
then this works for me too.
>> 
>> Fred
>> 
>>> 
>>> -- Sean Silva
>>>  
>>> 
>>> Thanks!
>>> Fred
>>> 
>>>> -- Sean Silva
>>>> 
>>>>  
>>>> 
>>>> Fred
>>>> 
>>>>>  
>>>>> 
>>>>> 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 <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/97cddfd4/attachment.html>