llvm dev - Sep 2016 - [ThinLTO] Importing based on PGO data

Hi,
I am working right now on importing based on PGO/FDO data. There is one
issue that I found - when we calculate the list of imports, we can't get
the ProfileSummaryInfo, which is the best and I
think only valid way of checking if callsite/callee is hot (isHotCount()).
There are 2 solutions that I come up with Teresa and Easwaran:

1. Add PGO data to summary
2. Replace CalleeInfo::ProfileCount with enum {None, Cold, Hot} computed
during computing summary.

I like the 2. much more. It will reduce the summary size slightly and I
don't think we will need ProfileCount anywhere else.

The other thing I would like to mention is that I think we should start
using the summary versioning and drop support of old version.
ThinLTO doesn't have enough users right now and parsing many versions of
summary will just add additional cost, that will start to grow.

Piotr
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160902/5938f2af/attachment.html>

On Fri, Sep 2, 2016 at 2:58 PM, Piotr Padlewski
<piotr.padlewski at gmail.com> wrote:
> Hi,
> I am working right now on importing based on PGO/FDO data. There is one
> issue that I found - when we calculate the list of imports, we can't
get the
> ProfileSummaryInfo, which is the best and I
> think only valid way of checking if callsite/callee is hot (isHotCount()).
> There are 2 solutions that I come up with Teresa and Easwaran:
>
> 1. Add PGO data to summary
> 2. Replace CalleeInfo::ProfileCount with enum {None, Cold, Hot} computed
> during computing summary.

Don't we already have edge profile count in the callgraph summary?
I think what is missing is the Profile SUmmary data itself -- that one
should be copied over to thinLTO summary so that the importing
analysis can use. However I we should not need to duplicate the
information in every module.

David

>
> I like the 2. much more. It will reduce the summary size slightly and I
> don't think we will need ProfileCount anywhere else.
>
> The other thing I would like to mention is that I think we should start
> using the summary versioning and drop support of old version.
> ThinLTO doesn't have enough users right now and parsing many versions
of
> summary will just add additional cost, that will start to grow.
>
> Piotr

On Fri, Sep 2, 2016 at 2:58 PM, Piotr Padlewski <piotr.padlewski at
gmail.com>
wrote:
> Hi,
> I am working right now on importing based on PGO/FDO data. There is one
> issue that I found - when we calculate the list of imports, we can't
get
> the ProfileSummaryInfo, which is the best and I
> think only valid way of checking if callsite/callee is hot (isHotCount()).
> There are 2 solutions that I come up with Teresa and Easwaran:
>
> 1. Add PGO data to summary
>
How much PGO data would need to be copied to summary?

> 2. Replace CalleeInfo::ProfileCount with enum {None, Cold, Hot} computed
> during computing summary.
>
It depends on how binary the inlining decisions that (will) use this are -
do we want to give different thresholds to medium hot/cold vs very hot/cold?

>
> I like the 2. much more. It will reduce the summary size slightly and I
> don't think we will need ProfileCount anywhere else.
>
> The other thing I would like to mention is that I think we should start
> using the summary versioning and drop support of old version.
> ThinLTO doesn't have enough users right now and parsing many versions
of
> summary will just add additional cost, that will start to grow.
>
It was released in 3.9 and in Xcode so it has external users and I believe
will need to support old versions going forward.

Teresa

>
> Piotr
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160902/e3d7648c/attachment.html>

2016-09-02 15:04 GMT-07:00 Xinliang David Li <davidxl at google.com>:
> On Fri, Sep 2, 2016 at 2:58 PM, Piotr Padlewski
> <piotr.padlewski at gmail.com> wrote:
> > Hi,
> > I am working right now on importing based on PGO/FDO data. There is
one
> > issue that I found - when we calculate the list of imports, we
can't get
> the
> > ProfileSummaryInfo, which is the best and I
> > think only valid way of checking if callsite/callee is hot
> (isHotCount()).
> > There are 2 solutions that I come up with Teresa and Easwaran:
> >
> > 1. Add PGO data to summary
> > 2. Replace CalleeInfo::ProfileCount with enum {None, Cold, Hot}
computed
> > during computing summary.
>
>
> Don't we already have edge profile count in the callgraph summary?
> I think what is missing is the Profile SUmmary data itself -- that one
> should be copied over to thinLTO summary so that the importing
> analysis can use. However I we should not need to duplicate the
> information in every module.
>
> David
>
> Yes we do have edge profile cout, but in order to compare it with global
couts we need Profile Summary as you said.
If we will follow 2) then we won't have to duplicate the data.

> >
> > I like the 2. much more. It will reduce the summary size slightly and
I
> > don't think we will need ProfileCount anywhere else.
> >
> > The other thing I would like to mention is that I think we should
start
> > using the summary versioning and drop support of old version.
> > ThinLTO doesn't have enough users right now and parsing many
versions of
> > summary will just add additional cost, that will start to grow.
> >
> > Piotr
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160902/baa7df7c/attachment.html>

2016-09-02 15:11 GMT-07:00 Teresa Johnson <tejohnson at google.com>:
>
>
> On Fri, Sep 2, 2016 at 2:58 PM, Piotr Padlewski <piotr.padlewski at
gmail.com
> > wrote:
>
>> Hi,
>> I am working right now on importing based on PGO/FDO data. There is one
>> issue that I found - when we calculate the list of imports, we
can't get
>> the ProfileSummaryInfo, which is the best and I
>> think only valid way of checking if callsite/callee is hot
>> (isHotCount()). There are 2 solutions that I come up with Teresa and
>> Easwaran:
>>
>> 1. Add PGO data to summary
>>
>
> How much PGO data would need to be copied to summary?
>
AFAI got information form Easwaran - not much, about 3*15 integers.
>
>
>> 2. Replace CalleeInfo::ProfileCount with enum {None, Cold, Hot}
computed
>> during computing summary.
>>
>
> It depends on how binary the inlining decisions that (will) use this are -
> do we want to give different thresholds to medium hot/cold vs very
hot/cold?
>
We could also just later add MediumHot, MediumCold to the enum. But I am
not sure if we want to have that specific information. We can always mark
medium hot
as hot, and then maybe import more stuff, which should not be that harrmful.

>
>>
>> I like the 2. much more. It will reduce the summary size slightly and I
>> don't think we will need ProfileCount anywhere else.
>>
>> The other thing I would like to mention is that I think we should start
>> using the summary versioning and drop support of old version.
>> ThinLTO doesn't have enough users right now and parsing many
versions of
>> summary will just add additional cost, that will start to grow.
>>
>
> It was released in 3.9 and in Xcode so it has external users and I believe
> will need to support old versions going forward.
>
Is this really needed? I think it is important to be able to distinguish
between versions, so combining old summary with new one won't lead to some
weird behaviour and user will get an error. The user will maybe have to
recompile whole project, but on the other hand we won't spend
time on trying to figure out support for all the versions.


>
> Teresa
>
>
>>
>> Piotr
>>
>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
> 408-460-2413
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160902/705d8702/attachment.html>