[LLVMdev] Inline hint for methods defined in-class

[Hal Finkel]

----- Original Message -----
> From: "Xinliang David Li" <davidxl at google.com>
> To: "Chandler Carruth" <chandlerc at gmail.com>
> Cc: cfe-commits at cs.uiuc.edu, "<llvmdev at cs.uiuc.edu> List" <llvmdev at cs.uiuc.edu>
> Sent: Wednesday, July 8, 2015 12:25:18 AM
> Subject: Re: [LLVMdev] Inline hint for methods defined in-class
> 
> On Tue, Jul 7, 2015 at 6:06 PM, Chandler Carruth
> <chandlerc at gmail.com> wrote:
> > On Tue, Jul 7, 2015 at 4:11 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.
> >
> >
> > First off, thanks for collecting the numbers and broadening the
> > distribution. Also, sorry it took me so long to get over to this
> > thread.
> >
> > I want to lay out my stance on this issue at a theoretical and
> > practical
> > level first. I'll follow up with thoughts on the numbers as well
> > after that.
> >
> > I think that tying *any* optimizer behavior to the 'inline' keyword
> > is
> > fundamentally the wrong direction.
> 
> Chandler, thanks for sharing your thought -- however I don't think it
> is wrong, let alone 'fundamentally wrong'.  Despite all the analysis
> that can be done, the inliner is in the end heuristic based. In lack
> of the profile data, when inlining two calls yield the same static
> benefit and size cost, it is reasonable for the inliner to think the
> call to the function with inline hint to yield more high
> dynamic/runtime benefit -- thus it has a higher static size budget to
> burn.
> 
> >We have reasons why we have done this
> > historically, and we can't just do an immediate about face, but we
> > should be
> > actively looking for ways to *reduce* the optimizer's reliance on
> > this
> > keyword to convey any meaning whatsoever.
> 
> yes those additional things will be done, but they are really
> orthogonal.
> 
> >
> > The reason I think that is the correct direction is because, for
> > better or
> > worse, the 'inline' keyword in C++ is not about optimization, but
> > about
> > linkage.
> 
> It is about both optimization and linkage. In fact the linkage simply
> serves as an implementation detail. In C++ standard 7.1.2,  paragraph
> 2 says:

The fact that C++ combines, into one keyword, a change in semantics (linkage) and an optimization hint is quite unfortunate. I wish it were otherwise. However, as it stands, I support this change. The benchmark numbers are encouraging, and it replaces an implementation quirk with the underlying (unfortunate) language design choice. The implementation quirk is that putting the inline keyword on an in-class function definition changes the behavior of the optimizer. However, according to the language specification, that definition should have implied that keyword. While an implementation is certainly free to do arbitrary things with hints, this behavior violates the spirit of the language specification. It makes a meaningless use of a standardized keyword meaningful, and that's the greater transgression. In addition, it does tend to be the case that in-class function definitions are small and suitable for inlining.

> 
> "A function declaration (8.3.5, 9.3, 11.3) with an inline specifier
> declares an inline function. The inline specifier indicates to the
> implementation that inline substitution of the function body at the
> point of call is to be preferred to the usual function call
> mechanism.
> An implementation is not required to perform this inline substitution
> at the point of call; however, even if this inline substitution is
> omitted, the other rules for inline functions defined by 7.1.2 shall
> still be respected."
> 
> Developers see those and rely on those to give compiler the hints.
> 
> Most importantly, paragraph 3 says:
> 
> "A function defined within a class definition is an inline function.
> The inline specifier shall not appear on a block scope function
> declaration.93 If the inline specifier is used in a friend
> declaration, that declaration shall be a definition or the function
> shall have previously been declared inline."
> 
> Here we can see regardless of how optimizer will honor the hint and
> to
> what extent, and based on what analysis,
> it is basically incorrect to drop the attribute on the floor for
> in-class function definitions. Eswaran's fix is justified with this
> reason alone.  The side effect of changing inliner behavior is
> irrelevant.
> 
> > It has a functional impact and can be both necessary or impossible
> > to use to meet those functional requirements. This in turn leaves
> > programmers in a lurch if the functional requirements are ever in
> > tension
> > with the optimizer requirements.
> 
> Not sure what you mean. Performance conscious programmers use it all
> the time.
> 
> >
> > We're also working really hard to get more widely deployed
> > cross-module
> > optimization strategies, in part to free programmers from the
> > requirement
> > that they put all their performance critical code in header files.
> > That
> > makes compilation faster, and has lots of benefits to the factoring
> > and
> > design of the code itself. We shouldn't then create an incentive to
> > keep
> > things in header files so that they pick up a hint to the
> > optimizer.
> 
> >
> > Ultimately, the world will be a better place if we can eventually
> > move code
> > away from relying on the hint provided by the 'inline' keyword to
> > the
> > optimizer.
> >
> 
> While I would like to see that happen some day, I do think  it is an
> independent matter.
> 
> >
> > That doesn't mean that the core concept of hinting to the optimizer
> > that a
> > particular function is a particularly good candidate for inlining
> > is without
> > value.
> 
> yes.
> 
> >While I think it is a bad practice that we shouldn't encourage in
> > code (especially portable code)
> 
> yes -- there are indeed programmers who use this casually without
> considering performance.
> 
> > I can see the desire to at least have *some*
> > attribute which is nothing more or less than a hint to the
> > optimizer to
> > inline harder[1].
> 
> yes -- there are programmers who use the attribute consciously.
> 
> > It would help people work around inliner bugs in the short
> > term, and even help debug inliner-rooted optimization problems.
> 
> I think it is a good hint to the compiler even in the longer term.
> With PGO, we should minimize the reliance on the hint though.
> 
> >Codebases
> > with strong portability requirements could still (and probably
> > should)
> > forbid or tightly control access to this kind of hint. I would want
> > really
> > strong documentation about how this attribute *completely voids*
> > your
> > performance warranty (if such a thing exists) as from version to
> > version of
> > the compiler it may go from a helpful hint to a devastatingly bad
> > hint.
> 
> Why? If the compiler becomes smarter and smarter, the inline hint
> will
> become more and more irrelevant and eventually has no effect -- why
> would the performance warranty be voided? If the compiler is not yet
> smart enough, why would the compiler refuse to take the hint and
> forbid developer provide the hint?
> 
> > But
> > I think I could be persuaded to live with such a hint existing. But
> > I'm
> > *really* uncomfortable with it being tied to something that also
> > impacts
> > linkage or other semantics of the program.
> 
> For consistent with standard, we should pass the attribute. Linkage
> is
> not affected in anyway.
> 
> >
> > [1]: Currently, the only other hint we have available is pretty
> > terrible as
> > it *also* has semantic effects: the always_inline attribute.
> >
> >
> >>
> >> 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
> >
> >
> > FWIW, this size increase seems *really* bad. I think that kills
> > this
> > approach even in the short term.
> 
> Re. size and performance trade-off -- 0.9% performance improvement
> should greatly win the size cost. Besides among all programs see,
> only
> clang sees this size increase with all the others seeing negligible
> size increase.

I agree. A 1% performance increase is worth a 4% code-size increase when not optimizing for size.

 -Hal

> 
> This is not a short term vs long term situation. It is basically a
> bug
> fix that FE drops the attribute. If it exposes inliner heuristic bug,
> that should be fixed/tuned separately.  With the hint correctly
> passed
> in, Easwaran will do further tuning including time based analysis.
> 
> >
> >>
> >> - Chrome: no performance improvement, 0.24% size increase
> >> - Google internal benchmark suite (geomean of ~20 benchmarks):
> >> ~1.8%
> >> performance improvement, no size regression
> >
> >
> > I'm also somewhat worried about the lack of any performance
> > improvements
> > outside of the Google benchmarks. That somewhat strongly suggests
> > that our
> > benchmarks are overly coupled to this hint already. The fact that
> > neither
> > Chrome, Clang, nor SPEC improved is... not at all encouraging.
> 
> Other than Google benchmarks, we do see Clang improve performance.
> Besides, current inliner needs to be further tuned in order to get
> more performance benefit. Passing the hint through is simply an
> enabler.  Also remember that most of SPEC benchmarks are C programs.
> C++ programs with heavy use of virtual functions may not benefit a
> lot
> either.
> 
> David
> 
> 
> >
> > -Chandler
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory