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