On Thu, Jul 9, 2015 at 12:49 PM, Nico Weber <tha...@chromium.org> wrote: > 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?
The sizes I reported are text sizes. This increases the text size by ~100K. The actual unstripped binary size actually decreases - likely due to smaller symbol table as outline copies get eliminated. If binary size is what you care about, this is a positive change for chrome :) Any heuristic based optimization is bound to negatively affect some application even if it improves a vast majority of cases. Also, to put this in perspective, if we set the inlinehint-threshold to inline-threshold (equivalent to removing the hint for functions with inline keyword), there is no change in performance* and the text size reduces by ~150K from the current baseline. *I am running the page_cycler.typical_25 benchmark and using the change in cold and warm times as the performance metric. > >> >> - 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