[LLVMdev] Inline hint for methods defined in-class

Xinliang David Li davidxl at google.com
Thu Jul 9 13:04:11 PDT 2015 

On Thu, Jul 9, 2015 at 12:49 PM, Nico Weber <thakis at chromium.org> wrote:
> On Tue, Jul 7, 2015 at 4:07 PM, Easwaran Raman <eraman at google.com> wrote:
>>
>> I'm reviving this thread after a while and CCing cfe-commits as
>> suggested by David Blaikie. I've also collected numbers building
>> chrome (from chromium, on Linux) with and without this patch as
>> suggested by David. I've re-posted the proposed patch and
>> performance/size numbers collected at the top to make it easily
>> readable for those reading it through cfe-commits.
>>
>> The proposed patch will add InlineHint to methods defined inside a class:
>>
>> --- a/lib/CodeGen/CodeGenFunction.cpp
>> +++ b/lib/CodeGen/CodeGenFunction.cpp
>> @@ -630,7 +630,7 @@ void CodeGenFunction::StartFunction(GlobalDecl GD,
>>    if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D)) {
>>      if (!CGM.getCodeGenOpts().NoInline) {
>>        for (auto RI : FD->redecls())
>> -        if (RI->isInlineSpecified()) {
>> +        if (RI->isInlined()) {
>>            Fn->addFnAttr(llvm::Attribute::InlineHint);
>>            break;
>>          }
>>
>> Here are the performance and size numbers I've collected:
>>
>>
>> - C++ subset of Spec: No performance effects, < 0.1% size increase
>> (all size numbers are text sizes returned by 'size')
>> - Clang: 0.9% performance improvement (both -O0 and -O2 on a large .ii
>> file) , 4.1% size increase
>> - Chrome: no performance improvement, 0.24% size increase
>
>
> This is probably relative to a nonstripped linux release build? So this
> means adding, what, 300kB to binary size without any benefit?

Passing the hint through is not the end goal. It is an information
(about the source construct and user intention) that backend deserves
to know to enable more analysis based tuning selectively.  In the end,
I hope inliner can be tuned to make everybody happy, but that is not
always possible. For those who care about both performance and size,
PGO is the way to go.

David



>
>>
>> - Google internal benchmark suite (geomean of ~20 benchmarks): ~1.8%
>> performance improvement, no size regression
>>
>> If there is any other important benchmark/application that needs to be
>> evaluated, I'll work on that.
>>
>> The main skepticism in this thread is about whether a developer
>> intends/expects a method defined in-class to be inlined or purely uses
>> size of the method body to make this decision. I'll let CFE developers
>> chime in on this. But irrespective of the intention, I think the data
>> suggests this is a useful signal in some good cases and has a small
>> size penalty in some bad cases. Note that if the criterion for placing
>> it in-class is purely based on size, and assuming the inline-threshold
>> is chosen to inline "small" functions, this change should only affect
>> a small number of functions (in the inline-threshold to
>> inlinehint-threshold range) and the risk of serious size bloat is low.
>>
>> - Easwaran
>>
>> On Wed, Jun 24, 2015 at 3:15 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> > On Wed, Jun 24, 2015 at 3:04 PM, Easwaran Raman <eraman at google.com>
>> > wrote:
>> >>
>> >> On Wed, Jun 24, 2015 at 2:35 PM, David Blaikie <dblaikie at gmail.com>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Wed, Jun 24, 2015 at 2:20 PM, Easwaran Raman <eraman at google.com>
>> >> > wrote:
>> >> >>
>> >> >> On Wed, Jun 24, 2015 at 2:10 PM, Robinson, Paul
>> >> >> <Paul_Robinson at playstation.sony.com> wrote:
>> >> >> >> -----Original Message-----
>> >> >> >> From: Easwaran Raman [mailto:eraman at google.com]
>> >> >> >> Sent: Wednesday, June 24, 2015 1:27 PM
>> >> >> >> To: Xinliang David Li
>> >> >> >> Cc: Robinson, Paul; Xinliang David Li; <llvmdev at cs.uiuc.edu> List
>> >> >> >> Subject: Re: [LLVMdev] Inline hint for methods defined in-class
>> >> >> >>
>> >> >> >> The method to identify functions with in-class definitions is one
>> >> >> >> part
>> >> >> >> of my question. Even if there is a way to do that without passing
>> >> >> >> the
>> >> >> >> hint, I'm interested in getting feedback on treating it at-par
>> >> >> >> with
>> >> >> >> functions having the inline hint in inline cost analysis.
>> >> >> >
>> >> >> > Well, personally I think having the 'inline' keyword mean "try
>> >> >> > harder"
>> >> >> > is worth something, but that's intuition backed by no data
>> >> >> > whatsoever.
>> >> >> > Your patch would turn 'inline' into noise, when applied to a
>> >> >> > function
>> >> >> > with an in-class definition.  Granted that the way the C++
>> >> >> > standard
>> >> >> > describes 'inline' it is effectively noise in that situation.
>> >> >>
>> >> >> The reason I started looking into this is that, for a suite of
>> >> >> benchmarks we use internally, treating the in-class definitions
>> >> >> equivalent to having an 'inline' keyword, when combined with a
>> >> >> higher
>> >> >> inlinehint-threshold, is a measurable win in performance. I am not
>> >> >> making any claim that this is a universal truth, but intuitively,
>> >> >> the
>> >> >> description of 'inline' in C++ standard seems to influence what
>> >> >> methods are defined in-class.
>> >> >
>> >> >
>> >> > I'm not sure that's the case - in my experience (for my own code &
>> >> > the
>> >> > code
>> >> > I see from others) people put stuff in headers that's "short enough"
>> >> > that
>> >> > it's not worth the hassle of an external definition. I don't really
>> >> > think
>> >> > authors are making an actual judgment about how much of a win
>> >> > inlining
>> >> > their
>> >> > function is most of the time when they put a definition inline in a
>> >> > class.
>> >> > (maybe a litttle more likely when it's a standalone function where
>> >> > you
>> >> > have
>> >> > to write "inline" explicitly, but maybe not even then)
>> >> Ok, that may very well be the case.
>> >>
>> >> > It would seem that improving the inliner to do a better job of
>> >> > judging
>> >> > the
>> >> > inlining benefit would be ideal (for this case and for LTO, etc -
>> >> > where
>> >> > we'll pick up equivalently small non-inline function definitions that
>> >> > the
>> >> > author had decided to define out of line (either because they used to
>> >> > be
>> >> > longer or the author didn't find out of line definitions to be as
>> >> > inconveniently verbose as someone else, etc)), if there's something
>> >> > more
>> >> > useful to go on than "the user sort of maybe implied that this would
>> >> > be
>> >> > good
>> >> > to inline". It seems like a very weak signal.
>> >>
>> >> I don't disagree with your ideal scenario. In the current non-ideal
>> >> state, LLVM does use a larger threshold for using the 'inline'
>> >> keyword. The question is whether using this larger threshold for
>> >> in-class definitions is a backward step.
>> >
>> >
>> > Probably worth having this conversation on cfe-commits (as it's a Clang
>> > change and Clang developers are likely to have a better feel for how C++
>> > developers use inline definitions).
>> > Might want to rope in Chrome developers too - they are very sensitive to
>> > size increases.
>> >
>> > & prototyping with the change to filter out templates would be relevant,
>> > of
>> > course.
>> >
>> > I don't see large-scale numbers (eg: across Google's perf benchmarks
>> > overall?) - spec is a bit narrow (& tends towards C code, if I'm not
>> > mistaken, so isn't likely to show much about this change), and that it
>> > improves the benchmark you were trying to improve would need to be
>> > weighed
>> > against the changes to a broader sample, I would think?
>>
>> >
>> > - David
>> >
>> >>
>> >>
>> >> - Easwaran
>> >>
>> >> > - David
>> >> >
>> >> >>
>> >> >>
>> >> >> - Easwaran
>> >> >>
>> >> >> > --paulr
>> >> >> >
>> >> >> >>
>> >> >> >> Thanks,
>> >> >> >> Easwaran
>> >> >> >>
>> >> >> >>
>> >> >> >> On Wed, Jun 24, 2015 at 12:56 PM, Xinliang David Li
>> >> >> >> <xinliangli at gmail.com> wrote:
>> >> >> >> > The problem is that the other way around is not true: a
>> >> >> >> > function
>> >> >> >> > linkonce_odr linkage may be neither inline declared nor have
>> >> >> >> > in-class
>> >> >> >> > definition.
>> >> >> >> >
>> >> >> >> > David
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > On Wed, Jun 24, 2015 at 11:53 AM, Robinson, Paul
>> >> >> >> > <Paul_Robinson at playstation.sony.com> wrote:
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> > -----Original Message-----
>> >> >> >> >> > From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-
>> >> >> >> bounces at cs.uiuc.edu]
>> >> >> >> >> > On
>> >> >> >> >> > Behalf Of Easwaran Raman
>> >> >> >> >> > Sent: Wednesday, June 24, 2015 9:54 AM
>> >> >> >> >> > To: Xinliang David Li
>> >> >> >> >> > Cc: <llvmdev at cs.uiuc.edu> List
>> >> >> >> >> > Subject: Re: [LLVMdev] Inline hint for methods defined
>> >> >> >> >> > in-class
>> >> >> >> >> >
>> >> >> >> >> > Ping.
>> >> >> >> >> >
>> >> >> >> >> > On Wed, Jun 17, 2015 at 4:13 PM, Xinliang David Li
>> >> >> >> <davidxl at google.com>
>> >> >> >> >> > wrote:
>> >> >> >> >> > > that looks like a different fix. The case mentioned by
>> >> >> >> >> > > Easwaran
>> >> >> >> >> > > is
>> >> >> >> >> > >
>> >> >> >> >> > > class A{
>> >> >> >> >> > >    int foo () { return 1; }
>> >> >> >> >> > >   ...
>> >> >> >> >> > > };
>> >> >> >> >> > >
>> >> >> >> >> > > where 'foo' is not explicitly declared with 'inline'
>> >> >> >> >> > > keyword.
>> >> >> >> >> > >
>> >> >> >> >> > > David
>> >> >> >> >> > >
>> >> >> >> >> > > On Wed, Jun 17, 2015 at 4:07 PM, Balaram Makam
>> >> >> >> <bmakam at codeaurora.org>
>> >> >> >> >> > wrote:
>> >> >> >> >> > >> AFAIK, this was fixed in r233817.
>> >> >> >> >>
>> >> >> >> >> That was later reverted.
>> >> >> >> >>
>> >> >> >> >> > >>
>> >> >> >> >> > >> -----Original Message-----
>> >> >> >> >> > >> From: llvmdev-bounces at cs.uiuc.edu
>> >> >> >> >> > >> [mailto:llvmdev-bounces at cs.uiuc.edu]
>> >> >> >> >> > On
>> >> >> >> >> > >> Behalf Of Easwaran Raman
>> >> >> >> >> > >> Sent: Wednesday, June 17, 2015 6:59 PM
>> >> >> >> >> > >> To: llvmdev at cs.uiuc.edu
>> >> >> >> >> > >> Cc: David Li
>> >> >> >> >> > >> Subject: [LLVMdev] Inline hint for methods defined
>> >> >> >> >> > >> in-class
>> >> >> >> >> > >>
>> >> >> >> >> > >> Clang adds the InlineHint attribute to functions that are
>> >> >> >> explicitly
>> >> >> >> >> > marked
>> >> >> >> >> > >> inline, but not if they are defined in the class body. I
>> >> >> >> >> > >> tried
>> >> >> >> >> > >> the
>> >> >> >> >> > following
>> >> >> >> >> > >> patch, which I believe handles the in-class definition
>> >> >> >> >> > >> case:
>> >> >> >> >> > >>
>> >> >> >> >> > >> --- a/lib/CodeGen/CodeGenFunction.cpp
>> >> >> >> >> > >> +++ b/lib/CodeGen/CodeGenFunction.cpp
>> >> >> >> >> > >> @@ -630,7 +630,7 @@ void
>> >> >> >> >> > >> CodeGenFunction::StartFunction(GlobalDecl
>> >> >> >> >> > >> GD,
>> >> >> >> >> > >>    if (const FunctionDecl *FD =
>> >> >> >> >> > >> dyn_cast_or_null<FunctionDecl>(D))
>> >> >> >> {
>> >> >> >> >> > >>      if (!CGM.getCodeGenOpts().NoInline) {
>> >> >> >> >> > >>        for (auto RI : FD->redecls())
>> >> >> >> >> > >> -        if (RI->isInlineSpecified()) {
>> >> >> >> >> > >> +        if (RI->isInlined()) {
>> >> >> >> >> > >>            Fn->addFnAttr(llvm::Attribute::InlineHint);
>> >> >> >> >> > >>            break;
>> >> >> >> >> > >>          }
>> >> >> >> >> > >>
>> >> >> >> >> > >> I tried this on C++ benchmarks in SPEC 2006. There is no
>> >> >> >> noticeable
>> >> >> >> >> > >> performance difference and the maximum text size increase
>> >> >> >> >> > >> is
>> >> >> >> >> > >> <
>> >> >> >> 0.25%.
>> >> >> >> >> > >> I then built clang with and without this change. This
>> >> >> >> >> > >> increases
>> >> >> >> the
>> >> >> >> >> > text
>> >> >> >> >> > >> size by 4.1%.  For measuring performance, I compiled a
>> >> >> >> >> > >> large
>> >> >> >> >> > >> (4.8
>> >> >> >> >> > million
>> >> >> >> >> > >> lines) preprocessed file. This change improves runtime
>> >> >> >> >> > >> performance
>> >> >> >> by
>> >> >> >> >> > 0.9%
>> >> >> >> >> > >> (average of 10 runs) in O0 and O2.
>> >> >> >> >> > >>
>> >> >> >> >> > >> I think knowing whether a function is defined inside a
>> >> >> >> >> > >> class
>> >> >> >> >> > >> body
>> >> >> >> is
>> >> >> >> >> > >> a
>> >> >> >> >> > >> useful hint to the inliner. FWIW, GCC's inliner doesn't
>> >> >> >> differentiate
>> >> >> >> >> > these
>> >> >> >> >> > >> from explicit inline functions. If the above results
>> >> >> >> >> > >> doesn't
>> >> >> >> justify
>> >> >> >> >> > this
>> >> >> >> >> > >> change, are there other benchmarks that I should
>> >> >> >> >> > >> evaluate?
>> >> >> >> >> > >> Another
>> >> >> >> >> > >> possibility is to add a separate hint for this instead of
>> >> >> >> >> > >> using
>> >> >> >> the
>> >> >> >> >> > existing
>> >> >> >> >> > >> inlinehint to allow for better tuning in the inliner.
>> >> >> >> >>
>> >> >> >> >> A function with an in-class definition will have linkonce_odr
>> >> >> >> >> linkage,
>> >> >> >> >> so it should be possible to identify such functions in the
>> >> >> >> >> inliner
>> >> >> >> >> without introducing the inlinehint attribute.
>> >> >> >> >> --paulr
>> >> >> >> >>
>> >> >> >> >> > >>
>> >> >> >> >> > >> Thanks,
>> >> >> >> >> > >> Easwaran
>> >> >> >> >> > >> _______________________________________________
>> >> >> >> >> > >> LLVM Developers mailing list
>> >> >> >> >> > >> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> >> >> >> >> > >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> >> >> >> >> > >>
>> >> >> >> >> > _______________________________________________
>> >> >> >> >> > LLVM Developers mailing list
>> >> >> >> >> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> >> >> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> >> >> >> >>
>> >> >> >> >> _______________________________________________
>> >> >> >> >> LLVM Developers mailing list
>> >> >> >> >> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> >> >> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> >> >> >> >
>> >> >> >> >
>> >> >> _______________________________________________
>> >> >> LLVM Developers mailing list
>> >> >> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> >> >
>> >> >
>> >
>> >
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>

More information about the llvm-dev mailing list