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

Thu Jul 20 11:19:50 PDT 2023

arsenm added a comment.

In D74162#4518301 <https://reviews.llvm.org/D74162#4518301>, @gchatelet wrote:

> The backend decides that this intrinsic is best implemented by calling libc's `memcpy` and inserts a call.

I looked into this and it's just a straight SelectionDAG bug. DAG.getMemcpy tries expanding inline depending on target size thresholds and custom instructions, but ultimately unconditionally emits an illegal libcall. It needs to consider the function availability. I've already "fixed" this particular issue in 3c848194f28decca41b7362f9dd35d4939797724 <https://reviews.llvm.org/rG3c848194f28decca41b7362f9dd35d4939797724>. This is an IR pass to work around SelectionDAG limitations, the DAG cannot directly emit a loop. Really the DAG builder code should emit a hard error if the memcpy runtime call isn't available. There is a small gap where unhandled variable sized memcpys could be introduced between PreISelIntrinsicLowering and isel, but this is just a SelectionDAG implementation issue. That would only be an issue for a hypothetical pass which were to introduce a variable sized memcpy in the late codegen pipeline, which I can't see a need for. GlobalISel doesn't have the same limitation, and we could always come up with a post-isel expanded pseudo hack to deal with it.

I think part of the fundamental problem is we've jammed two different concepts into a confusing set of overlapping control mechanisms. We currently have this set of attributes:

  `builtin`
  `nobuiltin`
  "no-builtins"
  "no-builtin-<func>"

I'm unclear on exactly the semantics of any of these is supposed to be. "no-builtins" isn't in the LangRef, and the wording on `nobuiltin` is oddly call site specific. With this confusion, I think the implementation is just buggy. What is the difference between `nobuiltin` and "no-builtins" supposed to be? The `nobuiltin` LangRef merely states it may be placed on declarations and definitions, but doesn't state what that means in that case.

There are 2 different concepts here: what calls the compiler is allowed to introduce, and which the compiler recognizes as having known semantics. There are traces of evidence that they're supposed to be separate but this wasn't fully implemented. We separately have "RuntimeLibcalls" and "TargetLibraryInfo", which have some overlap in the set of functions. Both are unfortunately hardcoded sets the target has to opt-out of manually. TargetLibraryInfo tries to respect "no-builtins", but no equivalent mechanism is used for "RuntimeLibcalls". As such, I do think the PreISelIntrinsicLowering case should be using some kind of runtime libcall information, though not necessarily TargetLibraryInfo.

> Now for `memcpy` in particular we have introduced `__builtin_memcpy_inline` (doc <https://clang.llvm.org/docs/LanguageExtensions.html#guaranteed-inlined-copy>) which allows us to convey the semantics of "fixed size operations that always generate code". They are guaranteed to never "call to the libc" (godbolt <https://godbolt.org/z/4f8M9zqq7>). Unfortunately those intrinsics are not supported in GCC so this would be clang only. Also they don't yet cover `memcmp` and `bcmp`.

I do not believe memcpy inline should be necessary. It should always be correct to emit an llvm.memcpy call anywhere, and the backend can always expand it. It needs to consider runtime call target availability, which is just not considered currently as a bug.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74162