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

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>

On Wed, May 25, 2016 at 2:17 PM, Xinliang David Li <davidxl at google.com>
wrote:
>
>
> 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?
>
Yes; actually for all functions. If I understand correctly, the InLTO
argument fixes a specific case of a general problem: the PGOFuncName is
currently constructed via an algorithm that depends on the current state of
the module and the result can therefore change as the module is transformed.
To solve this, the idea is to only run the algorithm at a single point of
truth (the module as seen by prof-gen/prof-use) and freeze the result in
metadata. All other accesses to PGOFuncName simply read that metadata, so
there is no possibility for getting out of date.

Currently, it seems the meaning of the InLTO flag is basically "we are
accessing the PGOFuncName at a point that is not prof-gen/prof-use, so use
the metadata". I just want to make that clearer.

-- Sean Silva

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

On Wed, May 25, 2016 at 4:34 PM, Sean Silva <chisophugis at gmail.com>
wrote:
>
>
> On Wed, May 25, 2016 at 2:17 PM, Xinliang David Li <davidxl at
google.com>
> wrote:
>
>>
>>
>> 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?
>>
>
> Yes; actually for all functions. If I understand correctly, the InLTO
> argument fixes a specific case of a general problem: the PGOFuncName is
> currently constructed via an algorithm that depends on the current state of
> the module and the result can therefore change as the module is
transformed.
> To solve this, the idea is to only run the algorithm at a single point of
> truth (the module as seen by prof-gen/prof-use) and freeze the result in
> metadata. All other accesses to PGOFuncName simply read that metadata, so
> there is no possibility for getting out of date.
>
>
> Currently, it seems the meaning of the InLTO flag is basically "we are
> accessing the PGOFuncName at a point that is not prof-gen/prof-use, so use
> the metadata". I just want to make that clearer.
>
This problem is kind of special to value profiling in LTO mode. In this
mode, the transformation needs to be delayed (when all function bodies are
available) so we end up having compute the pgonames at two different points
with different program states.  Your suggestion of doing this
unconditionally will work but will require more memory at compile time.

David


>
> -- Sean Silva
>
>
>>
>> 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/28a89f3e/attachment.html>

> On May 25, 2016, at 4:34 PM, Sean Silva via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
> 
> On Wed, May 25, 2016 at 2:17 PM, Xinliang David Li <davidxl at
google.com <mailto:davidxl at google.com>> wrote:
> 
> 
> On Wed, May 25, 2016 at 12:12 PM, Sean Silva <chisophugis at gmail.com
<mailto:chisophugis at gmail.com>> wrote:
> 
> 
> On Mon, May 23, 2016 at 8:23 PM, Sean Silva <chisophugis at gmail.com
<mailto: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
<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 <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?
> 
> Yes; actually for all functions. If I understand correctly, the InLTO
argument fixes a specific case of a general problem: the PGOFuncName is
currently constructed via an algorithm that depends on the current state of the
module and the result can therefore change as the module is transformed.
> To solve this, the idea is to only run the algorithm at a single point of
truth (the module as seen by prof-gen/prof-use) and freeze the result in
metadata. All other accesses to PGOFuncName simply read that metadata, so there
is no possibility for getting out of date.
> 
> Currently, it seems the meaning of the InLTO flag is basically "we are
accessing the PGOFuncName at a point that is not prof-gen/prof-use, so use the
metadata". I just want to make that clearer.
To provide more details: specifically the transformations that require the
metadata are the one that lead to changing the "PGOFuncName", so it is
specifically 1) turning a symbol in to internal/private, and 2) straightforward
renaming (which happens when you LTO-link two files that both have a local
symbol with the same name).
Right now I believe the metadata is added specifically at the right place in the
pipeline and only to cover the minimal amount of symbols that need to have the
metadata (as David wrote, to avoid using extra memory).


-- 
Mehdi


> 
> -- Sean Silva
>  
> 
> 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
> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160525/9e77b164/attachment-0001.html>