On Tue, Jul 7, 2015 at 4:07 PM, Easwaran Raman <era...@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 <dblai...@gmail.com> wrote: > > > > > > On Wed, Jun 24, 2015 at 3:04 PM, Easwaran Raman <era...@google.com> > wrote: > >> > >> On Wed, Jun 24, 2015 at 2:35 PM, David Blaikie <dblai...@gmail.com> > wrote: > >> > > >> > > >> > On Wed, Jun 24, 2015 at 2:20 PM, Easwaran Raman <era...@google.com> > >> > wrote: > >> >> > >> >> On Wed, Jun 24, 2015 at 2:10 PM, Robinson, Paul > >> >> <paul_robin...@playstation.sony.com> wrote: > >> >> >> -----Original Message----- > >> >> >> From: Easwaran Raman [mailto:era...@google.com] > >> >> >> Sent: Wednesday, June 24, 2015 1:27 PM > >> >> >> To: Xinliang David Li > >> >> >> Cc: Robinson, Paul; Xinliang David Li; <llvm...@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 > >> >> >> <xinlian...@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_robin...@playstation.sony.com> wrote: > >> >> >> >> > >> >> >> >> > >> >> >> >> > >> >> >> >> > -----Original Message----- > >> >> >> >> > From: llvmdev-boun...@cs.uiuc.edu [mailto:llvmdev- > >> >> >> boun...@cs.uiuc.edu] > >> >> >> >> > On > >> >> >> >> > Behalf Of Easwaran Raman > >> >> >> >> > Sent: Wednesday, June 24, 2015 9:54 AM > >> >> >> >> > To: Xinliang David Li > >> >> >> >> > Cc: <llvm...@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 > >> >> >> <davi...@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 > >> >> >> <bma...@codeaurora.org> > >> >> >> >> > wrote: > >> >> >> >> > >> AFAIK, this was fixed in r233817. > >> >> >> >> > >> >> >> >> That was later reverted. > >> >> >> >> > >> >> >> >> > >> > >> >> >> >> > >> -----Original Message----- > >> >> >> >> > >> From: llvmdev-boun...@cs.uiuc.edu > >> >> >> >> > >> [mailto:llvmdev-boun...@cs.uiuc.edu] > >> >> >> >> > On > >> >> >> >> > >> Behalf Of Easwaran Raman > >> >> >> >> > >> Sent: Wednesday, June 17, 2015 6:59 PM > >> >> >> >> > >> To: llvm...@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 > >> >> >> >> > >> llvm...@cs.uiuc.edu http://llvm.cs.uiuc.edu > >> >> >> >> > >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >> >> >> >> > >> > >> >> >> >> > _______________________________________________ > >> >> >> >> > LLVM Developers mailing list > >> >> >> >> > llvm...@cs.uiuc.edu http://llvm.cs.uiuc.edu > >> >> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >> >> >> >> > >> >> >> >> _______________________________________________ > >> >> >> >> LLVM Developers mailing list > >> >> >> >> llvm...@cs.uiuc.edu http://llvm.cs.uiuc.edu > >> >> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >> >> >> > > >> >> >> > > >> >> _______________________________________________ > >> >> LLVM Developers mailing list > >> >> llvm...@cs.uiuc.edu http://llvm.cs.uiuc.edu > >> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >> > > >> > > > > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits