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

Fri Jun 23 15:51:18 PDT 2017

On Fri, Jun 23, 2017 at 12:41 AM, Chandler Carruth <chandlerc at gmail.com>
wrote:

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

Adding this is certainly easy, but the adoption of the new attributes may
be an issue due to portability concerns.

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

Agreed. In fact a lot of use cases of always-inline attribute (for
performance reasons) can be used as test cases for improving inline
analysis.

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

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

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

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

>
> 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/15e54e0c/attachment.html>