[LLVMdev] Inline hint for methods defined in-class

[Xinliang David Li]

> Evidently not, but I'm also not sure that 'unanimous consensus' is an appropriate standard.
>
>> and -- most embarrassingly -- seems to
>> completely miss out the *actual* semantic impact of inline
>> functions. [Chandler, we should get that FAQ entry fixed.]
>
> The problem is that it is not just *that* FAQ, Google also ranks this one pretty highly:
>
>   http://www.cprogramming.com/tips/tip/member-functions-are-inline-by-default-cincrement
>
> The same is true in plenty of books too. For example, from the first book Google book search finds for "inline member function": "Object-oriented Programming: Using C++ for Engineering and Technology" by Goran Svenk (2003), says, "To create an inline member function, it is only necessary to place the function's definition inside the class. It is not necessary to use the inline keyword in this case, as is the case with non-member functions. Some functions -- long functions, recursive functions, ... -- cannot be expanded inline. If a member function falls into one of these categories and its definition is placed within the body of the class, the compiler will usually issue a warning message and process the function as a non-inline member function."
>
> I don't particularly like this explanation, and the last statement may not be entirely correct, but the point relevant to our discussion is clear. There is a significant portion of the skilled C++ programming community that shares the view for which I'm advocating. And to be clear, I'm advocating this viewpoint because I believe it is the prevailing viewpoint among C++ programmers. The fact that these FAQs, books, etc. agree with me I simply take as evidence that it is the prevailing viewpoint.
>

yes -- developers (except for compiler developers) won't know the
differences between 'inline functions' vs 'inline specified
functions', not to mention the exact intention of the standard is
subject to debate.

David


> As I've said, I don't particularly like the current state of affairs -- it is particularly problematic for the creation of header-only libraries: If you wish to create a header-only library, and you follow reasonable coding practices, the functions that you define in the class definitions are likely to be small and the functions you define outside the class definitions are likely to be larger. This, I think, is the most readable approach. However, even if you don't put the inline keyword on the in-class definitions, you do need to put it explicitly on the (larger) out-of-class definitions to avoid ODR violation. This is exactly the opposite of what you want: those larger functions are precisely the ones likely to be less suitable for inlining.
>
> However, if we're trying to fix that problem by not treating in-class functions definitions the same way we treat other functions with the inline keyword, we're missing most of the problem, and creating a quirky implementation in the process. The real problem is large functions that need the inline keyword to avoid ODR violation. But for that, we need a separate solution.
>
>>
>>
>> And I think you're still missing the point: whether we agree that the
>> standard says there's an inline hint here is not even relevant, in
>> and of itself. We should base our inlining on whatever gives the
>> best results (by whatever criteria we judge those results); the
>> standard gives us no normative requirements in this regard.
>
> I agree, but the standard does set a user expectation in this regard. And we should have a really good reason to violate that expectation, and we don't. The data seems to suggest that we're currently not doing the optimal thing, and that aligning our inlining heuristic with that expectation yields better performance results. That having been said, the fact that this expectation comes directly from the standard, gives it significantly more weight than it might have otherwise. That's my point: I'm not saying that our current implementation is not conforming, but I'm saying that it is providing a sub-optimal user experience by not satisfying the expectations that users have in this regard; expectations that come directly from the standard.
>
> Thanks again,
> Hal
>
>>
>>
>> What’s the deal with inline functions: When the compiler
>> inline-expands a function call, the function’s code gets inserted...
>> There are several ways to designate that a function is inline, some
>> of which involve the inline keyword, others do not. No matter how
>> you designate a function as inline, it is a request that ...
>>
>> Is there another way to tell the compiler to make a member function
>> inline?: Yep: define the member function in the class body itself:
>> ... This is often more convenient than the alternative of defining
>> your inline functions outside the class body. However, although it
>> is easier on the person who writes the class, it is harder on all
>> the readers since it mixes what a class does (the external behavior)
>> with how it does it ...
>>
>> -Hal
>>
>>
>>
>>
>> > the terms are "declared with an inline specifier" and
>> > > "inline function"
>> > >
>> >
>> > To be pedantic, "declared with an inline specifier" does not seem
>> > to
>> > occur in the standard. The closest thing I see is 7.1.2p2, "A
>> > function declaration (8.3.5, 9.3, 11.3) with an inline specifier
>> > declares an inline function.", and that *defines* the term "inline
>> > function".
>> >
>> > >
>> > > In any case, the point is: it is not reasonable to use the
>> > > standard's
>> > > wording to justify when to provide an inline hint.
>> >
>> > It is perfectly reasonable; the standard contains a facility
>> > designed
>> > explicitly for this purpose, and it happens to be the facility
>> > under
>> > discussion.
>> >
>> > > Even if we agreed
>> > > that it said that any inline function is intended to undergo
>> > > inline
>> > > substitution, that is only a non-binding suggestion due to the
>> > > as-if
>> > > rule, and we are under no obligation to base our inlining
>> > > decision
>> > > on it.
>> > >
>> >
>> > Agreed.
>> >
>> > >
>> > >
>> > >
>> > > > Developers see those and rely on those to give compiler the
>> > > > hints.
>> > >
>> > >
>> > >
>> > > Likewise here, different developers have different expectations,
>> > > so
>> > > I
>> > > don't think we can use this argument to make the decision.
>> > >
>> > >
>> > > There seem to be two relevant factors that should affect our
>> > > decision:
>> > >
>> > >
>> > > 1) What signals best indicate that inlining is beneficial for
>> > > existing code? That is, which heuristics make us optimize better?
>> > > and
>> >
>> > It is not quite that simple. We also need to give a high-quality
>> > user
>> > experience, and that means taking advantage of language facilities
>> > designed to provide optimization hints in a consistent way. Now we
>> > can debate what consistent is... ;)
>> >
>> > > 2) Which signals are reasonable for us to use, given the current
>> > > and
>> > > expected future state of the C++ language? We don't want people
>> > > contorting their code in order to get it well-optimized (such as
>> > > moving functions out of their class definition because they turn
>> > > out
>> > > to be somewhat non-trivial, and inlining them is harmful),
>> > > and we
>> > > want to allow inline hints to be given in ways that are
>> > > orthogonal
>> > > to program semantics (the inline specifier is not good for this,
>> > > because it also carries real language semantics, not just an
>> > > optimization hint).
>> > >
>> >
>> > I agree this is an unfortunate combination of properties for
>> > 'inline'
>> > to have. But it was not done by accident, and the intent of the
>> > language seems to be not to give us the option of treating them
>> > separately and still provide a consistent user experience. We
>> > certainly *can* and still have a conforming implementation... we
>> > could also only consider the first 100 inline specifiers for
>> > hinting
>> > and ignore the rest. But, in my experience, users have expectations
>> > that inline functions are (at least more likely) to be inlined than
>> > other functions of similar size, and this includes functions
>> > defined
>> > in the class definition. Furthermore, the data seems to indicate
>> > that this is the right approach.
>> >
>> > I also don't want users to burden their code with the 'inline'
>> > keyword on nearly every (or every) in-class function definition
>> > because, even though defining it there is supposed to make it an
>> > inline function, that will make it a 'really inline' inline
>> > function.
>> >
>> > Thanks again,
>> > Hal
>> >
>> > >
>> > >
>> > >
>> > >
>> > > > Most importantly, paragraph 3 says:
>> > > >
>> > > > "A function defined within a class definition is an inline
>> > > > function.
>> > > >
>> > > >
>> > > >
>> > > > ... the same is *not* true for a function definition that
>> > > > appears
>> > > > within a class definition. That is merely an inline function
>> > > > (that
>> > > > is, it can be defined in multiple translation units).
>> > > >
>> > > >
>> > > > 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.
>> > > >
>> > > > 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
>> > > >
>> > > >
>> > > > _______________________________________________
>> > > > 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
>> > >
>> > >
>> >
>> > --
>> > Hal Finkel
>> > Assistant Computational Scientist
>> > Leadership Computing Facility
>> > Argonne National Laboratory
>> > _______________________________________________
>> > 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
>>
>>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory