Teresa Johnson via llvm-dev
2016-Sep-02 22:32 UTC
[llvm-dev] [ThinLTO] Importing based on PGO data
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>
Piotr Padlewski via llvm-dev
2016-Sep-02 22:37 UTC
[llvm-dev] [ThinLTO] Importing based on PGO data
2016-09-02 15:32 GMT-07:00 Teresa Johnson <tejohnson at google.com>:> > > 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. > >I don't know what is planned for the inliner. The Easwaran patch only uses isHotCount right now, but I think that the data that David mentioned should be better for future tunning ( and probably will take only a few bits). Piotr> 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/945c9ccf/attachment.html>
Easwaran Raman via llvm-dev
2016-Sep-02 22:44 UTC
[llvm-dev] [ThinLTO] Importing based on PGO data
IMO, multiple levels of hotness/coldness is not very useful. If that proves to be beneficial, tweaking the hotness cutoffs and the inline threshold for hot/cold sites should get to similar performance on average without the additional complexity. On Fri, Sep 2, 2016 at 3:32 PM, Teresa Johnson <tejohnson at google.com> wrote:> > > 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/9272f503/attachment.html>
Xinliang David Li via llvm-dev
2016-Sep-02 22:55 UTC
[llvm-dev] [ThinLTO] Importing based on PGO data
On Fri, Sep 2, 2016 at 3:44 PM, Easwaran Raman <eraman at google.com> wrote:> IMO, multiple levels of hotness/coldness is not very useful. If that > proves to be beneficial, tweaking the hotness cutoffs and the inline > threshold for hot/cold sites should get to similar performance on average > without the additional complexity. > >I think it can be useful -- even though the inliner's implementation may choose to not use the information. Suppose we have compile time/import size budget, having more fine grained information helps the importer prioritize the callsites based on that information -- similar to priority based inliner. Besides I don't see how it adds more complexity. David> On Fri, Sep 2, 2016 at 3:32 PM, Teresa Johnson <tejohnson at google.com> > wrote: > >> >> >> 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/9512b624/attachment.html>