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

Chandler Carruth via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 23 00:41:50 PDT 2017


On Thu, Jun 22, 2017 at 10:43 PM Xinliang David Li via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
>
> On Thu, Jun 22, 2017 at 9:14 PM, Chandler Carruth via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> chandlerc added a comment.
>>
>> In https://reviews.llvm.org/D34471#788736, @davidxl wrote:
>>
>> > Chandler's example does not involve any indirect calls so it is not
>> related to this patch.
>>
>>
>> Actually, it is...
>>
>> The original test case doesn't look exactly like my example. Rather than
>> a template argument and a call through that template argument, it uses an
>> indirect call to a normal function argument.
>>
>> My suggestion is that the code should be changed to use a template
>> argument if the intent is to make `always_inline` actually *always* inline.
>>
>>
> There are situations where using template does not apply.
>

I understand that there are hypothetical situations, but the only concrete
use case I'm aware of isn't one of them... If there are other benchmarks
where this matters, I'd be interested in which ones.

However, the fact that there might be other interesting cases is why I
brought up an explicit attribute, I just don't think we should handle it in
this way. See my more detailed response below, as I think that's the more
interesting discussion.

I don't think this is really the right way to model the `always_inline`
>> attribute. I continue to think that this attribute should mean that we very
>> literally *always* inline. See the discussion here for more details:
>> http://lists.llvm.org/pipermail/llvm-dev/2015-August/089466.html
>
>
> I don't disagree with this.  In reality, always_inline is not used in that
> sense literally -- solving that problem (fixing user sources) is a
> different problem, especially when the alternative way to suggest a very
> strong inline hint does not yet exist.
>

But we could easily add it if it is in fact needed. I would rather do that
than add more logic around always inline.

Let's assume that we *do* actually add a strong inline hint, and the rest
of this discussion is about that attribute, because I agree that is a much
more interesting scenario, and you're raising important points for that
scenario...

(Note that I *also* am still in favor of having an attribute that is
>> actually a very strong hint to do inlining, I just don't think any of the
>> existing attributes really fit that bill,
>
>
> True, and always_inline is used (wrongly) to fit that bill.
>
>
>> and I don't think the original motivating test case is a particularly
>> compelling example. Even if a user *does* want to get really strong
>> inlining because of a weird indirect function call, I don't know why the
>> inliner has to go to great lengths to detect this rather than expecting the
>> user to also attribute the function receiving the function pointer...)
>>
>>
> Firstly, The user/(indirect) caller of the function (marked as always
> inline) and the function itself are likely written by different authors.
> The library function provider may mark the function with the attribute, and
> the client/user may not even aware of the attribute (and the benefit of
> inlining it) -- only the compiler/inliner knows about it, so it is not the
> job of the client code to mark the caller to be always inline.
>

While this is true, I tend to agree with Eli that this still has a really
unacceptable risk of rapid and unexpected code growth.

Also, I think that at a somewhat fundamental level this misses the point of
this kind of attribute. See below, as the purpose of having this kind of
strong hint I think is relevant to several of these issues.


>
> Secondly, marking the caller (of an indirect callee with always_inline
> attribute) always_inline is usually not the right way to go. The right way
> is to mark the callsite as always inline.
>

I absolutely agree that at least in some cases this needs to be applied to
call sites. That's one advantage of this being a new attribute, we can work
to design it in a way that supports this kind of use case.

I still think there are reasons to want this on a function definition in
some cases, but definitely not all.


>
> Thirdly, the motivation of this patch is not to 'implement' inlining of
>  indirect calls to always inline functions, but as an inline heuristics for
> better performance.  I guess there will be no objection to the patch if the
> attribute checked is not 'always_inline' but the  new very strong inline
> hint Chandler suggested.  In other words, this is something inliner needs
> to do one way or the other.
>

So, I think a key thing here is what the purpose of these kinds of
attributes are...

I primarily view the utility of a strong inline hint as a way to work
around some combination of
1) weaknesses in the inliner itself
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)
3) an inability to get a profile or use static hints

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.

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.

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.


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.

-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170623/a2442dbd/attachment.html>


More information about the llvm-commits mailing list