[PATCH] D24940: [thinlto] Add cold-callsite import heuristic

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D24940#554135, @Prazek wrote:

> In https://reviews.llvm.org/D24940#553351, @eraman wrote:
>
> > How does this result in improved performance? If the
> >
> > In https://reviews.llvm.org/D24940#553322, @mehdi_amini wrote:
> >
> > > > Not tunned up heuristic, but with this small heuristic there is about +0.10% improvement on SPEC 2006
> > >
> > >
> > > This is not clear to me: I would expect only a compile time impact on this, why is there a perf improvement?
> >
> >
> > The inliner and the importer use a different metric to determine what is cold: the importer uses the ProfleSummaryInfo while the inliner checks if the entry count is less than a fraction of max_entry_count (the inliner's code is the one that needs to change, but that's dependent on porting inliner to the new PM) This would explain the performance improvement (inliner normally inlines functions that are considered cold as determined by PSI.isColdCount() but prevented from doing so if they aren't imported). If we were to fix the inliner first, then this change should be  nop in terms of performance.
>
>
> Well, I think there is another reason - if I won't import function, that is called only from cold block, then it won't gonna be inlined, where the inliner in the trunk doesn't have any heuristic on NOT inlining functions to cold block
>  (it have heuristic to not inline functions to block ending with unreachable, but it is not connected to PGO). So by not importing some functions I prevent inliner from inlining stuff that it shouldn't.


It is possible, but realistically 0.10% improvement is not enough to draw any strong conclusions.

Overall looks fine, will re-review when you've addressed Mehdi's comments.


https://reviews.llvm.org/D24940