[PATCH] D34471: [Inliner] Boost inlining of an indirect call to always_inline function.

[Chandler Carruth via llvm-commits]

Just to close the loop on one of the open questions here (seems we have a
reasonable path forward generally):

On Fri, Jun 23, 2017 at 3:51 PM Xinliang David Li <davidxl at google.com>
wrote:

> On Fri, Jun 23, 2017 at 12:41 AM, Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
>
>> 2) weaknesses in the design of the code (making it impossible to get
>> reasonable performance without heroics of inlining or impossible for a
>> profile to produce the necessary inlining hints)
>>
>
> I don't see how this prevents profile to produce necessary inlining hints
> -- but I do see this part missing (using profile to estimate benefit of
> indirect call inlining) from the current inliner.
>

I've seen code that essentially forces (unnecessarily) very hot and very
cold to code paths that are different to share a large region of code, so
that it is hard to get the profile information needed into the right place.
Essentially, a very deep call stack which is almost entirely the same
between hot path and cold path so that the call site which is actually
"hot" and distinguishes all of this is very far away from the thing we need
to optimize.

But this seems really rare and maybe not important / a distraction. Sorry
for that. I've seen it only once in a parser where a pile of parsing logic
was shared between hot and cold paths and it made it hard to get a hot-path
optimized version because *so much* code had to be specialized. It ended up
being hard to manage the code in that form as well (different parsing
approaches were desired in the hot path) so it made sense to fix the source
code design eventually.


>
>
>> 3) an inability to get a profile or use static hints
>>
>>
> by' ... to get a profile', you mean the inability of the user to use PGO?
>

Yeah.


>
>
>
>> Or something along these lines. Essentially, it's an escape hatch when
>> other things that *should* allow the inliner to work aren't enough. I
>> absolutely think it is a necessary thing to have, but I think this should
>> guide how it works.
>>
>>
> The static hints available today are indeed not enough. For instance 'hot'
> attribute applies to the function, not the callsite. builtin_expect applies
> to a branch locally, and is not enough to indicate the importance of a
> callsite either.
>

Completely agree. Very happy to see more work designing a more expressive
and useful set of source annotations. Especially ones that carry clear
enough semantics that programmers both can use them when needed and also
*don't* use them when *bad*. Historically, the latter has been an even
bigger problem than the former.


>
>> For example, I think it is actually reasonable for this attribute to be
>> very localized and *not* impact the thresholds w.r.t. indirect calls in
>> other functions. The reason is that it seems like it should both be as
>> simple of a heuristic adjustment and localized as possible.
>>
>
> With the current inline design, a function level inline related attribute
> will need affect thresholds of all callsites, thus won't be 'localized' in
> order to be effective, or do you mean something else?
>

This gets back to the point you make which I totally agree with --
eventually we really need the 'strong inline hint' annotation to be
available *both* on a function but also on a *call*.

While we could productively start with a quick version that was a
function-level attribute, I think we would want to as an immediate next
step (and pretty clear plan at the beginning) extend it to calls so that it
can be effective in cases where a function level annotation is too broad.


>
>>
>> I also think this is important for *users* of a library to be aware of --
>> these use cases don't make a lot of sense as some fundamental part of the
>> abstractions of a library that are hidden from the users of the library.
>>
>>
> This hidden information does not affect library interfaces but just the
> underlying implementation, so the users probably don't care.
>
>>
>> All of this said, it is possible that we'll encounter real-world use
>> cases for a strong attribute hint that require it to have a complex
>> interaction with inlining heuristics like what it in this patch. But as the
>> only real-world case I know of is easily addressed with a template, I'd
>> prefer to wait to have this complexity in the inline cost heuristics based
>> on some case where we really can't solve or work around the problem in any
>> other way. The inliner's heuristics are already complex, and so it seems
>> good to avoid adding more here.
>>
>>
>>
>>> We should implement this performance heuristic for always inline
>>> indirect calls now, and when the new attribute is available, emit warnings
>>> about the wrong use of always inline and deprecate this eventually.
>>>
>>
>> Generally, unless this is going to take some very large body of technical
>> work, it seems like we should just start off with the correct long term
>> design. For example, just adding a function definition attribute that
>> conveys the desired meaning should be quite easy. The design and
>> implementation of call-site annotations can be done separately and wouldn't
>> block this. But hopefully all of this is moot given that there are source
>> workarounds for the practical cases that I know of and we might be able to
>> take a different approach for any future issues that arise.
>>
>
> ok, that sounds fine.
>
> David
>
>
>>
>> -Chandler
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170623/0f4f9546/attachment.html>