[LLVMdev] Inline hint for methods defined in-class

[Hal Finkel]

----- Original Message -----
> From: "Chandler Carruth" <chandlerc at gmail.com>
> To: "Xinliang David Li" <davidxl at google.com>, "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: Thursday, July 9, 2015 12:29:09 AM
> Subject: Re: [LLVMdev] Inline hint for methods defined in-class
> 
> On Tue, Jul 7, 2015 at 10:27 PM Xinliang David Li <
> davidxl at google.com > wrote:
> 
> 
> 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.
> 
> 
> 
> This is an argument for having *some* source level hinting construct.
> While I think such a construct is very risky, I did actually suggest
> that we add such a hint because I recognize some of the practical
> necessities for it.
> 
> 
> My primary argument is against using the 'inline' keyword as the
> source of the hint, and especially using the happenstance of a
> method body in a class as the source of the hint.
> 
> 
> 
> 
> >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.
> 
> 
> 
> I'm not talking about additional things, I'm talking about separating
> the optimization hint from the semantics and linkage changing
> constructs. That does not seem 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:
> 
> "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.
> 
> 
> 
> This is essentially a nonsense paragraph for a standardized
> specification of a programming language. How hard you optimize code
> doesn't have any bearing on the conformance and behavior of the
> program. =/
> 
> 
> I think this paragraph is part of the historical context. I think we
> should change C++ to remove it (and I will propose that change) and
> I think we should change C++ to support a standardized
> *non*-semantic hint if folks really want to see that in the form of
> a C++11-style [[attribute]].
> 

I don't understand exactly what you propose to change? I doubt you'll be able to convince the committee to make the 'inline' keyword not imply the optimization hint. I also doubt that you'll be able to remove the linkage-semantics attached to the 'inline' keyword, because that would be a massively-breaking change. The only thing you might be able to do is to remove the fact that in-class definitions imply the optimization hint part of 'inline'. This is practical, in a sense, but I highly suspect you'll run into exactly this problem because every vendor will run benchmarks and assert that making that change will, in general, degrade performance (for exactly the same reasons it would generally help performance with LLVM). This is not a random result, it is because...

> 
> I'm also really, really confident that most developers are not using
> the wording of the standard as the basis of how they tune the
> performance of their code. =/
> 

I'm really confident you're wrong. Almost all, even moderately-experienced, C++ developers with whom I've worked explicitly make the decision on what code to put in the class definition and what not to put in the class definition on the understanding that putting the function definition in the class is a hint/instruction to the compiler to inline it. Most don't really understand about the linkage changes in detail, and frankly, that's just a work-around to make it possible for this scheme to work. But they certainly do know that putting functions in the class definition should make the compiler inline them.

And the other issue underlying this is that the language design decision does not seem random either. It makes a lot of sense, putting small function definitions in the class definitions tends to enhance readability, but putting large ones in the class definition tends to harm it. Small functions that do simple things, which are the kinds of functions for which readability is enhanced by having them in the class definition, tend to also be good inlining candidates. Thus, it is not even clear that it is helpful to the average programmer to change this rule.

Please do feel free to bring up this issue on the EWG mailing list, I'll be happy to be wrong about this (because, as I've said, this current arrangement is highly suboptimal), but in the mean time, I disagree with you.

How about we get this change in controlled by a feature flag so that we can all do further experiments, and encourage our users to do so, more easily?

 -Hal

> 
> 
> 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.
> 
> 
> 
> I don't really understand what you're saying. Clang correctly carries
> all of the *semantics* required for in-class method bodies. We
> simply don't attach an optimization hint? I don't think this is
> "incorrect". Nothing in the standard says how hard we should try
> (and it can't, which is why the standard doesn't make sense to give
> advice here).
> 
> 
> 
> 
> 
> > 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.
> 
> 
> 
> You might *have* to use the inline keyword to get correct linkage
> even when it causes the optimizer to inline and that *hurts*
> performance.
> 
> 
> Similarly, you might *have* to not use the inline keyword to get
> correct linkage even though you would like to give the optimizer a
> hint for performance reasons (and are doing LTO, so you can).
> 
> 
> Essentially, there is no guarantee that the semantic requirements of
> linkage are correctly aligned with the desired optimizer hint.
> 
> 
> 
> > 
> > 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.
> 
> 
> 
> Adding further optimization hints based around the linkage further
> entrenches that model. If we want to move in this direction, this
> patch is a step in the *wrong* direction.
> 
> 
> 
> >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?
> 
> 
> 
> I didn't suggest the compiler would do anything. I suggested that the
> idea of hinting an optimizer about how to inline code is inherently
> non-portable (its specific to an optimizer) and thus would likely be
> less used in code bases with unusually strong portability concerns.
> 
> 
> 
> 
> > 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.
> 
> 
> 
> I do not think that there is any standards-consistency argument for
> one optimization hint over another. We already get the linkage
> correct for all of these things. The only consideration is the
> *degree* to which we prefer to actually do inlining in the
> optimizer. The standard at no point makes guarantees about these
> degrees.
> 
> 
> 
> > 
> > [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.
> 
> 
> 
> But the other programs don't see any performance wins.
> 
> 
> We can likely get this 1% of runtime performance without paying that
> high of a size cost. This is a real concern -- there are uses of
> Clang and LLVM that are very sensitive to size regressions. And I
> suspect we'll see other applications that also see the size loss.
> 
> 
> If we could more *selectively* use a dedicated hint to get the
> performance boost, we'd almost certainly not have to give up this
> much code size.
> 
> 
> > 
> >> 
> >> - 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.
> 
> 
> 
> I don't really understand why that changes my point... Are you saying
> that without this hint, we can't do the subsequent work on the
> inliner?
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> 

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