[LLVMdev] Inline hint for methods defined in-class (original) (raw)

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 = dyncastornull(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 >> >> <PaulRobinson 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 >> >> >> > linkonceodr linkage may be neither inline declared nor have >> >> >> > in-class >> >> >> > definition. >> >> >> > >> >> >> > David >> >> >> > >> >> >> > >> >> >> > On Wed, Jun 24, 2015 at 11:53 AM, Robinson, Paul >> >> >> > <PaulRobinson 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 = >> >> >> >> > >> dyncastornull(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 linkonceodr >> >> >> >> 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/llvm-dev/attachments/20150709/2e48f574/attachment.html>

More information about the llvm-dev mailing list