llvm dev - Nov 2019 - Fix clang's 'flatten' function attribute: add depth to always_inline?

Hi everyone,

clang currently implements the 'flatten' function attribute by marking
all calls to not 'noinline' functions with 'always_inline'. In
effect,
only the first level of calls is inlined, not all calls recursively
(like gcc does).

We briefly discussed possible solutions on IRC:

We could add an equivalent LLVM attribute for functions (e.g.
'flatten'). The main problem with this approach occurs when we're
inlining the function marked with 'flatten'. In this case we could
either drop the attribute, move it to the function we're inlining into
(both would lose the scope of the original 'flatten' annotation), or
distribute it to all calls within the 'flatten' function (which would
require a new call site attribute).

The other approach is to add or modify a call site attribute. We could
add a specific attribute (e.g. 'flatten' or
'always_inline_recursively'), but a more general solution is adding a
new 'depth' parameter to the existing 'always_inline' attribute.
When a
call site marked 'always_inline' is inlined, the attribute will then be
duplicated to all new call sites (with decremented depth and only if the
depth is greater than zero).

With this solution, one problem remains: an 'always_inline' on the call
site is currently stronger than a 'noinline' on the callee. Thus, a
recursive 'always_inline' would ignore 'noinline'. To fix this,
we can
make 'always_inline' weaker than 'noinline'. If that breaks
backwards
compatibility too much, we could add a second parameter (boolean 'weak'
or 'strong') to 'always_inline'.

I've never worked on LLVM before, but if someone confirms what the
preferred solution to this is, I will start working on a patch.

Thanks for your time
LevitatingLion

I like the proposal to add a new call site attribute, either "flatten"
or
"always_inline_recursively".

I wouldn't want to add an optional depth to the existing always_inline
attribute. It already has a well understood meaning.

Good luck. :)

On Mon, Nov 4, 2019 at 2:12 PM LevitatingLion via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi everyone,
>
> clang currently implements the 'flatten' function attribute by
marking
> all calls to not 'noinline' functions with 'always_inline'.
In effect,
> only the first level of calls is inlined, not all calls recursively
> (like gcc does).
>
> We briefly discussed possible solutions on IRC:
>
> We could add an equivalent LLVM attribute for functions (e.g.
> 'flatten'). The main problem with this approach occurs when
we're
> inlining the function marked with 'flatten'. In this case we could
> either drop the attribute, move it to the function we're inlining into
> (both would lose the scope of the original 'flatten' annotation),
or
> distribute it to all calls within the 'flatten' function (which
would
> require a new call site attribute).
>
> The other approach is to add or modify a call site attribute. We could
> add a specific attribute (e.g. 'flatten' or
> 'always_inline_recursively'), but a more general solution is adding
a
> new 'depth' parameter to the existing 'always_inline'
attribute. When a
> call site marked 'always_inline' is inlined, the attribute will
then be
> duplicated to all new call sites (with decremented depth and only if the
> depth is greater than zero).
>
> With this solution, one problem remains: an 'always_inline' on the
call
> site is currently stronger than a 'noinline' on the callee. Thus, a
> recursive 'always_inline' would ignore 'noinline'. To fix
this, we can
> make 'always_inline' weaker than 'noinline'. If that breaks
backwards
> compatibility too much, we could add a second parameter (boolean
'weak'
> or 'strong') to 'always_inline'.
>
> I've never worked on LLVM before, but if someone confirms what the
> preferred solution to this is, I will start working on a patch.
>
> Thanks for your time
> LevitatingLion
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191105/6f42170f/attachment.html>

(This is very loosely related, but the new attribute was something that
could be directly used by this patch for adding callsite-level inline
attributes from C/C++: https://reviews.llvm.org/D51200)

On Tue., Nov. 5, 2019, 13:25 Reid Kleckner via llvm-dev, <
llvm-dev at lists.llvm.org> wrote:
> I like the proposal to add a new call site attribute, either
"flatten" or
> "always_inline_recursively".
>
> I wouldn't want to add an optional depth to the existing always_inline
> attribute. It already has a well understood meaning.
>
> Good luck. :)
>
> On Mon, Nov 4, 2019 at 2:12 PM LevitatingLion via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Hi everyone,
>>
>> clang currently implements the 'flatten' function attribute by
marking
>> all calls to not 'noinline' functions with
'always_inline'. In effect,
>> only the first level of calls is inlined, not all calls recursively
>> (like gcc does).
>>
>> We briefly discussed possible solutions on IRC:
>>
>> We could add an equivalent LLVM attribute for functions (e.g.
>> 'flatten'). The main problem with this approach occurs when
we're
>> inlining the function marked with 'flatten'. In this case we
could
>> either drop the attribute, move it to the function we're inlining
into
>> (both would lose the scope of the original 'flatten'
annotation), or
>> distribute it to all calls within the 'flatten' function (which
would
>> require a new call site attribute).
>>
>> The other approach is to add or modify a call site attribute. We could
>> add a specific attribute (e.g. 'flatten' or
>> 'always_inline_recursively'), but a more general solution is
adding a
>> new 'depth' parameter to the existing 'always_inline'
attribute. When a
>> call site marked 'always_inline' is inlined, the attribute will
then be
>> duplicated to all new call sites (with decremented depth and only if
the
>> depth is greater than zero).
>>
>> With this solution, one problem remains: an 'always_inline' on
the call
>> site is currently stronger than a 'noinline' on the callee.
Thus, a
>> recursive 'always_inline' would ignore 'noinline'. To
fix this, we can
>> make 'always_inline' weaker than 'noinline'. If that
breaks backwards
>> compatibility too much, we could add a second parameter (boolean
'weak'
>> or 'strong') to 'always_inline'.
>>
>> I've never worked on LLVM before, but if someone confirms what the
>> preferred solution to this is, I will start working on a patch.
>>
>> Thanks for your time
>> LevitatingLion
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191106/efa32998/attachment-0001.html>

I have a working patch which adds the 'flatten' attribute almost ready,
but it breaks the Bitcode/highLevelStructure.3.2.ll test. This is caused by the
attribute definition in include/llvm/IR/Attributes.td:

def Flatten : EnumAttr<"flatten">;

llvm-dis seems to be unable to correctly decode attributes stored in the bitcode
when the new attribute is added. Why does this happen, and how do I fix it?

Other attributes don't seem to have required any handling of this problem,
see https://reviews.llvm.org/D62766 or https://reviews.llvm.org/D49165. Could
that be because this is the 65th attribute (so a bitmask indicating all
attributes doesn't fit in 64 bit anymore)?

On November 5, 2019 7:24:58 PM GMT+01:00, Reid Kleckner <rnk at
google.com> wrote:
>I like the proposal to add a new call site attribute, either
"flatten"
>or
>"always_inline_recursively".
>
>I wouldn't want to add an optional depth to the existing always_inline
>attribute. It already has a well understood meaning.
>
>Good luck. :)
>
>On Mon, Nov 4, 2019 at 2:12 PM LevitatingLion via llvm-dev <
>llvm-dev at lists.llvm.org> wrote:
>
>> Hi everyone,
>>
>> clang currently implements the 'flatten' function attribute by
>marking
>> all calls to not 'noinline' functions with
'always_inline'. In
>effect,
>> only the first level of calls is inlined, not all calls recursively
>> (like gcc does).
>>
>> We briefly discussed possible solutions on IRC:
>>
>> We could add an equivalent LLVM attribute for functions (e.g.
>> 'flatten'). The main problem with this approach occurs when
we're
>> inlining the function marked with 'flatten'. In this case we
could
>> either drop the attribute, move it to the function we're inlining
>into
>> (both would lose the scope of the original 'flatten'
annotation), or
>> distribute it to all calls within the 'flatten' function (which
would
>> require a new call site attribute).
>>
>> The other approach is to add or modify a call site attribute. We
>could
>> add a specific attribute (e.g. 'flatten' or
>> 'always_inline_recursively'), but a more general solution is
adding a
>> new 'depth' parameter to the existing 'always_inline'
attribute. When
>a
>> call site marked 'always_inline' is inlined, the attribute will
then
>be
>> duplicated to all new call sites (with decremented depth and only if
>the
>> depth is greater than zero).
>>
>> With this solution, one problem remains: an 'always_inline' on
the
>call
>> site is currently stronger than a 'noinline' on the callee.
Thus, a
>> recursive 'always_inline' would ignore 'noinline'. To
fix this, we
>can
>> make 'always_inline' weaker than 'noinline'. If that
breaks backwards
>> compatibility too much, we could add a second parameter (boolean
>'weak'
>> or 'strong') to 'always_inline'.
>>
>> I've never worked on LLVM before, but if someone confirms what the
>> preferred solution to this is, I will start working on a patch.
>>
>> Thanks for your time
>> LevitatingLion
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191112/862c9539/attachment.html>