[clang] [llvm] [clang/LLVM] Add flatten_deep attribute for depth-limited inlining (1/2) (PR #165777)

[Erich Keane via llvm-commits]

erichkeane wrote:

> > Also, needs a release note.
> > I didn't review anything in the LLVM directory--
> > I think the name of this is wrong, and that it is a function level attribute is perhaps the incorrect idea here. I also think that how this is being described isn't quite accurate.
> > We should ALSO describe what this means in cases where inlining isn't possible, or when inlining is otherwise necessary/beneficial for further depths.
> > Such as: What if you `flatten_depth(2)`, but the thing inlined at depth 2 is `always_inline`? We NEED to inline that, despite it being depth 3. Also, what if our cost modelling decides it is now beneficial for that? It seems to me it is a foot-gun to have hte users annotate their function, then be stuck not getting an inlining of something obvious/trivial because of this (consider a simple wrapper function).
> 
> Thank you for the detailed feedback! I appreciate the thoughtful review. Let me address your points: I agree that `flatten_depth` is a better name. Regarding the semantics: You're right that it's not strictly a "maximum depth" (since `always_inline` or other mechanisms could cause deeper inlining). It's also not a "minimum depth" - if the call tree doesn't have enough depth, we simply inline as far as possible. I propose we describe it as a "target depth" or "suggested depth" - it's the depth to which we _attempt_ to inline, subject to the usual inlining constraints and decisions. This would:
> 
>     * Prevent issues like infinite loops with recursive functions
> 
>     * Work cooperatively with other inlining attributes and cost modeling
> 
>     * Not be a hard constraint in either direction
> 

I'd suggest describing it as a 'requested depth' or something (or even a depth hint?), but otherwise yes, all of that needs to be VERY clear in the docs.

> 
> I'll update the release notes and documentation to clarify the behavior in edge cases:
> 
>     1. **`always_inline` at deeper depths**: These should still be honored - the attribute doesn't prevent deeper inlining when required or beneficial
> 
>     2. **Cost model overrides**: If the cost model determines deeper inlining is beneficial (e.g., trivial wrapper functions), that should proceed normally
> 
>     3. **Insufficient call depth**: The attribute simply inlines as far as the call tree allows
> 
> 
> The goal is to provide a _hint_ for aggressive inlining up to a certain depth, not a strict barrier.

Good, please make sure this is VERY clear in documentation.



> 
> Regarding function-level vs call-site annotation: I understand your point about call-site annotation. However, there's value in the function-level approach here:
> 
>     * **Consistency with `flatten`**: This attribute is an extension of the existing `flatten` attribute, which is already function-level
> 
>     * **`always_inline` precedent**: LLVM's `always_inline` can be applied both at function level and call site
> 
>     * **Use case difference**: Function-level annotation allows library authors to specify inlining behavior for their APIs without requiring every caller to annotate call sites
> 
> 
> That said, I'm open to supporting both function-level and call-site annotations (similar to how `always_inline` works) if that would make the feature more useful. Does this address your concerns? I'm happy to iterate on the semantics and documentation.

I think the function-level is much more fraught with difficulty to use.  I don't see it as particularly useful?  IF we're wanting to do BOTH (which I suspect I could disagree/commit on), I would be open to doing the call-site in a 2nd review if others are open to that.



https://github.com/llvm/llvm-project/pull/165777