[PATCH] D86025: [CodeGen] Respect libfunc availability when lowering intrinsic memcpy

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D86025#2222338 <https://reviews.llvm.org/D86025#2222338>, @arsenm wrote:

> The memcpy intrinsic has nothing to do with library availability, and handling it should not depend on the library function. The backend may choose to emit a libcall for it, but this is not required (e.g. AMDGPU has no libcalls whatsoever).

Yeah, that's kind of the behaviour I expected when building with `-fno-builtin`. But then again, as @efriedma pointed out, GCC also does generate similar memcpy libcalls despite `-fno-builtin` (it didn't in the cases I tested before, on x86_64, but I now checked more and found cases where it does as well), so I'm a bit more hesitant about actually proceeding with this now...

> GlobalISel should be able to handle arbitrary memcpys with ease since it's possible to emit a loop. With SelectionDAG you have to expand the arbitrary case in the IR, but it still handles constant size cases.

Ok, so where would I start out doing that, more concretely? (I have close to no experience in either transforms that change IR or GlobalISel.)



================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1787
+  if (ID == Intrinsic::memcpy && !LibInfo->has(LibFunc_memcpy))
+    return false;
   if (translateKnownIntrinsic(CI, ID, MIRBuilder))
----------------
arsenm wrote:
> This needs to be dropped. D85199 switches these to using proper legalizeable instructions. It may make more sense for the libcall action to fail if the libraryinfo does not have memcpy
The reason for placing it here, was that if we within `translateKnownIntrinsic` decide we don't want to handle it that way, and return false from that method, we just proceed further down here, instead of making this method return false (prompting the caller to handle it differently).

But it does indeed break cases where the target would have a different, non-libcall based lowering of the intrinsic...


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6438
   // use a (potentially long) sequence of loads and stores.
-  if (AlwaysInline) {
+  if (AlwaysInline || (!LibInfo->has(LibFunc_memcpy) && ConstantSize)) {
     assert(ConstantSize && "AlwaysInline requires a constant size!");
----------------
aemerson wrote:
> So if memcpy is not available but we have a non-constant size, then we still generate a memcpy call here?
Yeah, that's what the current patch does - that's clearly not ideal either...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86025