[LLVMdev] Inline hint for methods defined in-class

[Chandler Carruth]

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'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. =/


>
> 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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150709/608d05bc/attachment.html>