[PATCH] D74162: [Inliner] Inlining should honor nobuiltin attributes

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 added a comment.

In D74162#4514824 <https://reviews.llvm.org/D74162#4514824>, @tejohnson wrote:

> In D74162#4508033 <https://reviews.llvm.org/D74162#4508033>, @jhuber6 wrote:
>
>> I was discussing this patch with @arsenm after investigating why the calls to `libc` functions weren't being inlined on the GPU. What was the motivation for the original change? I figured it would make no difference if we inlined a function that doesn't allow built-ins into one that does. This is problematic because it prevents us from inlining any function from the LLVM C library under LTO. I'd like a way to at least work around this in the short term, but I'd like some context for why this was introduced.
>
> @gchatelet to add more context as this was related to work he was doing for custom implementations of libc memcpy functions.
>
> Context is in D61634 <https://reviews.llvm.org/D61634>. See specifically the discussion from https://reviews.llvm.org/D61634#1502201 to https://reviews.llvm.org/D61634#1512020 which discusses the inliner changes for the new attribute. IIRC the issue is that allowing such inlining would lose the no-builtin attributes, which would defeat their purpose.

Losing the ability to inline any function implemented in an LTO library compiled with `-ffreestanding` seems like a very bad tradeoff.  I was talking with @arsenm about this and the attribute seems undocumented and sparsely tested. What was the specific failure case that this was introduced to solve? We lose that attribute, but is that a bad thing? If we inline a function that cannot call builtins into a function that can, what is the issue with calling builtins at that point?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74162