[PATCH] D104475: [Clang][Codegen] emit noprofile IR Fn Attr for new Fn Attr no_profile

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D104475#2825841 <https://reviews.llvm.org/D104475#2825841>, @nickdesaulniers wrote:

> In D104475#2825795 <https://reviews.llvm.org/D104475#2825795>, @MaskRay wrote:
>
>> In D104475#2825772 <https://reviews.llvm.org/D104475#2825772>, @nickdesaulniers wrote:
>>
>>> In D104475#2825711 <https://reviews.llvm.org/D104475#2825711>, @MaskRay wrote:
>>>
>>>> So if we don't want to offer guarantee for IR/C/C++ attributes, we can document that users may need to additionally specify `__attribute__((noinline))` to suppress inlining.
>>>
>>> I don't generally like that approach:
>>
>> If a guarantee would cause inferior optimization or we cannot think of all the complication/fallout initially, I'd prefer that we keep the initial semantics narrow.
>
> The guarantee is more important than the optimization.
>
>> If we cannot make a promise, don't provide a promise.
>> No-suppressing inlining makes the attribute freely composable with other attributes (https://lists.llvm.org/pipermail/llvm-dev/2021-April/150062.html )
>
> IIUC, isn't @jdoerfert arguing that `optnone` and `noinline` being composable is _bad_?

No, the opposite. @jdoerfert is arguing that attributes should be composable.
I agree and I think the `optnone` design has some orthogonality problems.

>>> 1. it's not easy for developers to validate their call call chains to ensure that a caller with a restrictive function attribute doesn't have unrestricted callers inlined into the restricted caller.
>>> 2. that behavior opposes the principle of least surprise.
>>
>> Disagree. If the user wants a function not to be inlined, let them add `__attribute__((noinline))`.
>>
>>> 3. We don't have a statement attribute for call sites to say "please don't inline this call" which would be fine grain. noinline is a course-grain hammer; what if we want to inline a callee into most callers but not this one caller that has such a restricted function attribute?
>>
>>
>>
>>> See also D91816 <https://reviews.llvm.org/D91816> where I took the conservative approach for `no_stack_protector` and simply chose not to perform such inline substitution on caller and callee function attribute mismatch.  I find this behavior to be the least surprising, and the developer is provided with introspection as to why the compile did not perform such inlining via `-Rpass=inline` flag.
>>
>> I think D91816 <https://reviews.llvm.org/D91816> was an unfortunate case. That would require the inliner to understand every possible attribute.
>
> Hyperbole. Only a few such function attributes would need such restrictions.
>
>> That was the practical approach back then because it is not easy for the kernel to cope.
>> For new things we should strive for making the kernel do the right thing.
>>
>> For example, if a kernel function can sometimes be inlinable, sometimes not: add a alias and add `__attribute__((noinline))` to that alias.
>
> Interesting idea, but `noinline` is straight up broken in LLVM: https://godbolt.org/z/MoE1a7c3v. The Linux kernel relies on `noinline` meaning don't perform inline substitution.  That violates the principal of least surprise.  When a developer uses `noinline`, `no_stack_protector`, or `no_profile`, they mean DO NOT inline, insert a stack protector, or insert profiling/coverage instrumentation EVER.
>
> Also debugging to find out that inlining was the cause of such violated expectations sucks.

It is not broken. That is constant propagation in early-cse. A function call can be optimized out if it has no side effect and its value can be determined. The behavior may look like inlining. That is the `nointeropt` IR attribute proposal intends to address.
I guess for most(all?) non-trivial noinline usage in the Linux kernel, we can assume `noinline` is fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104475/new/

https://reviews.llvm.org/D104475