(original) (raw)

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(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

\- 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@gmail.com> wrote:
\>
\>
\> On Wed, Jun 24, 2015 at 3:04 PM, Easwaran Raman <eraman@google.com> wrote:
\>>
\>> On Wed, Jun 24, 2015 at 2:35 PM, David Blaikie <dblaikie@gmail.com> wrote:
\>> >
\>> >
\>> > On Wed, Jun 24, 2015 at 2:20 PM, Easwaran Raman <eraman@google.com>
\>> > wrote:
\>> >>
\>> >> On Wed, Jun 24, 2015 at 2:10 PM, Robinson, Paul
\>> >> <Paul\_Robinson@playstation.sony.com> wrote:
\>> >> >> -----Original Message-----
\>> >> >> From: Easwaran Raman \[mailto:eraman@google.com\]
\>> >> >> Sent: Wednesday, June 24, 2015 1:27 PM
\>> >> >> To: Xinliang David Li
\>> >> >> Cc: Robinson, Paul; Xinliang David Li; <llvmdev@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@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@playstation.sony.com> wrote:
\>> >> >> >>
\>> >> >> >>
\>> >> >> >>
\>> >> >> >> > -----Original Message-----
\>> >> >> >> > From: llvmdev-bounces@cs.uiuc.edu \[mailto:llvmdev-
\>> >> >> bounces@cs.uiuc.edu\]
\>> >> >> >> > On
\>> >> >> >> > Behalf Of Easwaran Raman
\>> >> >> >> > Sent: Wednesday, June 24, 2015 9:54 AM
\>> >> >> >> > To: Xinliang David Li
\>> >> >> >> > Cc: <llvmdev@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@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@codeaurora.org>
\>> >> >> >> > wrote:
\>> >> >> >> > >> AFAIK, this was fixed in r233817.
\>> >> >> >>
\>> >> >> >> That was later reverted.
\>> >> >> >>
\>> >> >> >> > >>
\>> >> >> >> > >> -----Original Message-----
\>> >> >> >> > >> From: llvmdev-bounces@cs.uiuc.edu
\>> >> >> >> > >> \[mailto:llvmdev-bounces@cs.uiuc.edu\]
\>> >> >> >> > On
\>> >> >> >> > >> Behalf Of Easwaran Raman
\>> >> >> >> > >> Sent: Wednesday, June 17, 2015 6:59 PM
\>> >> >> >> > >> To: llvmdev@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(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@cs.uiuc.edu http://llvm.cs.uiuc.edu
\>> >> >> >> > >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
\>> >> >> >> > >>
\>> >> >> >> > \_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_
\>> >> >> >> > LLVM Developers mailing list
\>> >> >> >> > LLVMdev@cs.uiuc.edu http://llvm.cs.uiuc.edu
\>> >> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
\>> >> >> >>
\>> >> >> >> \_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_
\>> >> >> >> LLVM Developers mailing list
\>> >> >> >> LLVMdev@cs.uiuc.edu http://llvm.cs.uiuc.edu
\>> >> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
\>> >> >> >
\>> >> >> >
\>> >> \_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_
\>> >> LLVM Developers mailing list
\>> >> LLVMdev@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