[LLVMdev] Inline hint for methods defined in-class

Nico Weber thakis at chromium.org
Thu Jul 9 12:49:17 PDT 2015


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?


> - 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150709/2e48f574/attachment.html>


More information about the cfe-commits mailing list