[LLVMdev] Inline hint for methods defined in-class

[Chandler Carruth]

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

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

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.


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. While I think it is a bad practice that we shouldn't
encourage in code (especially portable code) 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]. It would help people work around inliner
bugs in the short term, and even help debug inliner-rooted optimization
problems. 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. 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.

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


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

-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150708/240f2758/attachment.html>