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

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

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

I think this requires changes to the bitcode reader. This has been discussed a bit, see https://lists.llvm.org/pipermail/llvm-dev/2019-March/131216.html for example.