[LLVMdev] Inline hint for methods defined in-class

[Xinliang David Li]

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:

"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.

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