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

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>

On Fri, Sep 2, 2016 at 3:16 PM, Piotr Padlewski
<piotr.padlewski at gmail.com> wrote:
>
>
> 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.
Ok -- basically the profile summary is already consumed before
writing.  If you go this route, I think you need more enum values for
fine tuning: for instance hot_10 --> 10 percentile hotness, ... hot_90
etc, and cold_99, cold_999 etc.

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 3:30 PM, Xinliang David Li <davidxl at google.com>
wrote:
> On Fri, Sep 2, 2016 at 3:16 PM, Piotr Padlewski
> <piotr.padlewski at gmail.com> wrote:
> >
> >
> > 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.
>
> Ok -- basically the profile summary is already consumed before
> writing.  If you go this route, I think you need more enum values for
> fine tuning: for instance hot_10 --> 10 percentile hotness, ... hot_90
> etc, and cold_99, cold_999 etc.
>
Right, that relates to my question about how the inliner will eventually
use the same profile summary data, and whether it will be useful to
distinguish between various levels of hotness.

Teresa

> 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
> >
> >
>


-- 
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/83ad6150/attachment-0001.html>

> On Sep 2, 2016, at 3:16 PM, Piotr Padlewski <piotr.padlewski at
gmail.com> wrote:
> 
> 
> 
> 2016-09-02 15:04 GMT-07:00 Xinliang David Li <davidxl at google.com
<mailto:davidxl at google.com>>:
> On Fri, Sep 2, 2016 at 2:58 PM, Piotr Padlewski
> <piotr.padlewski at gmail.com <mailto: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.
Can you explain a bit more the issue here?

Thanks,

Mehid

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

The profile summary is saved in the global metadata ASAIK. If we want to
calculate if something is hot/cold while choosing functions for importing,
we would either need to read whole Module (which we clearly don't want to
do)
or duplicate this information in the summary, so we could get it without
reading Module.

2016-09-02 15:49 GMT-07:00 Mehdi Amini <mehdi.amini at apple.com>:
>
> On Sep 2, 2016, at 3:16 PM, Piotr Padlewski <piotr.padlewski at
gmail.com>
> wrote:
>
>
>
> 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.
>
>
> Can you explain a bit more the issue here?
>
> Thanks,
>
> Mehid
>
>
> 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/3a631bf9/attachment-0001.html>