PreISelIntrinsicLowering: don't expand memcpys in minsize functions, even with no-builtins.

[aemerson]

Summary

Even in the presence of -fno-builtin, users may directly call memcpy via the
builtin. In this case expanding will cause a size increase, so just avoid
expanding if the caller is a minsize function.

[arsenm]

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

201

Lowering the intrinsic when the libcall isn't available is mandatory, you can't simply skip it.

For minsize, you could adjust the default size threshold. Also, we could inject into the module an implementation you expand into, so the expansion happens fewer times

llvm/test/CodeGen/ARM/no-expand-memcpy-minsize-no-builtins.ll

10

I don't understand this, minsize can't override an explicit don't-do-this

[aemerson]

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

201

no-builtins is attached to the Function with -fno-builtins right? However the semantics of -fno-builtins is not AFAICT that we need to expand all memcpy intrinsics. The semantics are that clang shouldn't try to recognize things like memcpy loops and turn them into intrinsics. Or that plain memcpy calls shouldn't be converted into intrinsics.

[aemerson]

@efriedma @gchatelet do you happen to know what the expectation here is if we have a memcpy intrinsic with -fno-builtins/"no-builtins" attribute?

[arsenm]

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

201

This whole thing is a confusing mess, there's a thread about it going on over in D74162.

"no-builtins" isn't in the LangRef and I don't really understand how it's supposed to differ from nobuiltin.

I think we have 2 blurred concepts of which calls are recognized and which calls can be emitted. The call here should probably be considering "RuntimeLibcalls" not TLI, but that doesn't seem to be written down anywhere

[efriedma]

As far as I know, before 3c848194f28decca41b7362f9dd35d4939797724, things worked the following way:

• LLVM optimizations wouldn't introduce llvm.memcpy calls into nobuiltin functions.
• Any llvm.memcpy calls written explicitly in the input IR expand to "memcpy" (which we assume the user provides if necessary).

Maybe you could argue the latter behavior is a "bug", but it's behavior we provided for many years, and users depend on it. (And it's gcc-compatible.)

3c848194f28decca41b7362f9dd35d4939797724 changes things so that instead of calling "memcpy", we generate a bunch of terrible inline code. This apparently landed without review?

[arsenm]

In D155790#4526472, @efriedma wrote:

As far as I know, before 3c848194f28decca41b7362f9dd35d4939797724, things worked the following way:

• LLVM optimizations wouldn't introduce llvm.memcpy calls into nobuiltin functions.
• Any llvm.memcpy calls written explicitly in the input IR expand to "memcpy" (which we assume the user provides if necessary).

Maybe you could argue the latter behavior is a "bug", but it's behavior we provided for many years, and users depend on it. (And it's gcc-compatible.)

I've considered that the codegen pipeline can't deal with freestanding environments for fundamental intrinsics to be a major flaw.

I think the fix that avoids the current problem, and solves my desires is to move from checking TargetLibraryInfo.hasLibFunc(memcpy) to checking if TargetLoweringInfo::getLibcallName(memcpy) is valid (plus maybe some additional libcall-is-valid-for-addrspace checks?). Sorting out the general libcall mess for freestanding is a problem for another day

Independently of that, the lowering here could improve to optionally emit outlined implementations depending on target preference + minsize/optsize.

3c848194f28decca41b7362f9dd35d4939797724 changes things so that instead of calling "memcpy", we generate a bunch of terrible inline code.

This apparently landed without review?

This was D152383

[efriedma]

In D155790#4528021, @arsenm wrote:

I think the fix that avoids the current problem, and solves my desires is to move from checking TargetLibraryInfo.hasLibFunc(memcpy) to checking if TargetLoweringInfo::getLibcallName(memcpy) is valid (plus maybe some additional libcall-is-valid-for-addrspace checks?). Sorting out the general libcall mess for freestanding is a problem for another day

So conventional CPU targets use "memcpy", GPUs and other exotic targets use the new expansion codepath? That makes sense, I guess.

3c848194f28decca41b7362f9dd35d4939797724 changes things so that instead of calling "memcpy", we generate a bunch of terrible inline code.

This apparently landed without review?

This was D152383

Sorry, missed that.

[aemerson]

In D155790#4526472, @efriedma wrote:

As far as I know, before 3c848194f28decca41b7362f9dd35d4939797724, things worked the following way:

• LLVM optimizations wouldn't introduce llvm.memcpy calls into nobuiltin functions.
• Any llvm.memcpy calls written explicitly in the input IR expand to "memcpy" (which we assume the user provides if necessary).

Maybe you could argue the latter behavior is a "bug", but it's behavior we provided for many years, and users depend on it. (And it's gcc-compatible.)

3c848194f28decca41b7362f9dd35d4939797724 changes things so that instead of calling "memcpy", we generate a bunch of terrible inline code. This apparently landed without review?

In D155790#4528021, @arsenm wrote:

In D155790#4526472, @efriedma wrote:

As far as I know, before 3c848194f28decca41b7362f9dd35d4939797724, things worked the following way:

• LLVM optimizations wouldn't introduce llvm.memcpy calls into nobuiltin functions.
• Any llvm.memcpy calls written explicitly in the input IR expand to "memcpy" (which we assume the user provides if necessary).

Maybe you could argue the latter behavior is a "bug", but it's behavior we provided for many years, and users depend on it. (And it's gcc-compatible.)

I've considered that the codegen pipeline can't deal with freestanding environments for fundamental intrinsics to be a major flaw.

I think the fix that avoids the current problem, and solves my desires is to move from checking TargetLibraryInfo.hasLibFunc(memcpy) to checking if TargetLoweringInfo::getLibcallName(memcpy) is valid (plus maybe some additional libcall-is-valid-for-addrspace checks?). Sorting out the general libcall mess for freestanding is a problem for another day

Are you working on this? If not we'd like to get this fixed and have the previous behavior restored.

Independently of that, the lowering here could improve to optionally emit outlined implementations depending on target preference + minsize/optsize.

3c848194f28decca41b7362f9dd35d4939797724 changes things so that instead of calling "memcpy", we generate a bunch of terrible inline code.

This apparently landed without review?

This was D152383

[arsenm]

In D155790#4551830, @aemerson wrote:

I think the fix that avoids the current problem, and solves my desires is to move from checking TargetLibraryInfo.hasLibFunc(memcpy) to checking if TargetLoweringInfo::getLibcallName(memcpy) is valid (plus maybe some additional libcall-is-valid-for-addrspace checks?). Sorting out the general libcall mess for freestanding is a problem for another day

Are you working on this? If not we'd like to get this fixed and have the previous behavior restored.

No, I thought you were since you posted the patch

[aemerson]

In D155790#4551886, @arsenm wrote:

In D155790#4551830, @aemerson wrote:

I think the fix that avoids the current problem, and solves my desires is to move from checking TargetLibraryInfo.hasLibFunc(memcpy) to checking if TargetLoweringInfo::getLibcallName(memcpy) is valid (plus maybe some additional libcall-is-valid-for-addrspace checks?). Sorting out the general libcall mess for freestanding is a problem for another day

Are you working on this? If not we'd like to get this fixed and have the previous behavior restored.

No, I thought you were since you posted the patch

Ok got it.

[hiraditya]

llvm/test/CodeGen/ARM/no-expand-memcpy-minsize-no-builtins.ll

10

yeah, this could result in errors where there is no memcpy implementation.

[arsenm]

In D155790#4551945, @aemerson wrote:

In D155790#4551886, @arsenm wrote:

In D155790#4551830, @aemerson wrote:

I think the fix that avoids the current problem, and solves my desires is to move from checking TargetLibraryInfo.hasLibFunc(memcpy) to checking if TargetLoweringInfo::getLibcallName(memcpy) is valid (plus maybe some additional libcall-is-valid-for-addrspace checks?). Sorting out the general libcall mess for freestanding is a problem for another day

Are you working on this? If not we'd like to get this fixed and have the previous behavior restored.

No, I thought you were since you posted the patch

Ok got it.

I interpreted this as you were doing it but I did this in D157567

[aemerson]

In D155790#4575156, @arsenm wrote:

In D155790#4551945, @aemerson wrote:

In D155790#4551886, @arsenm wrote:

In D155790#4551830, @aemerson wrote:

I think the fix that avoids the current problem, and solves my desires is to move from checking TargetLibraryInfo.hasLibFunc(memcpy) to checking if TargetLoweringInfo::getLibcallName(memcpy) is valid (plus maybe some additional libcall-is-valid-for-addrspace checks?). Sorting out the general libcall mess for freestanding is a problem for another day

Are you working on this? If not we'd like to get this fixed and have the previous behavior restored.

No, I thought you were since you posted the patch

Ok got it.

I interpreted this as you were doing it but I did this in D157567

Yeah I was going to, I'm just on vacation so hadn't gotten around to it. Thanks!

[arsenm]

This can be abandoned

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

 // TODO: Handle atomic memcpy and memcpy.inline
 // TODO: Pass ScalarEvolution
 bool PreISelIntrinsicLowering::expandMemIntrinsicUses(Function &F) const {
   Intrinsic::ID ID = F.getIntrinsicID();
   bool Changed = false;
 
   for (User *U : llvm::make_early_inc_range(F.users())) {
     Instruction *Inst = cast<Instruction>(U);
+    // Even in the presence of -fno-builtin, users may directly call memcpy
+    // via the builtin. In this case expanding will cause a size increase,
+    // so just avoid expanding here if the caller is a minsize function.
+    bool IsMinSize = Inst->getFunction()->hasMinSize();
 
     switch (ID) {
     case Intrinsic::memcpy: {
       auto *Memcpy = cast<MemCpyInst>(Inst);
       Function *ParentFunc = Memcpy->getFunction();
       const TargetTransformInfo &TTI = LookupTTI(*ParentFunc);
-      if (shouldExpandMemIntrinsicWithSize(Memcpy->getLength(), TTI)) {
+      if (!IsMinSize &&
+          shouldExpandMemIntrinsicWithSize(Memcpy->getLength(), TTI)) {
         if (UseMemIntrinsicLibFunc &&
             LookupLibInfo(*ParentFunc).has(LibFunc_memcpy))
           break;
 
         expandMemCpyAsLoop(Memcpy, TTI);
         Changed = true;
         Memcpy->eraseFromParent();
       }
 
       break;
     }
     case Intrinsic::memmove: {
       auto *Memmove = cast<MemMoveInst>(Inst);
       Function *ParentFunc = Memmove->getFunction();
       const TargetTransformInfo &TTI = LookupTTI(*ParentFunc);
-      if (shouldExpandMemIntrinsicWithSize(Memmove->getLength(), TTI)) {
+      if (!IsMinSize &&
+          shouldExpandMemIntrinsicWithSize(Memmove->getLength(), TTI)) {
         if (UseMemIntrinsicLibFunc &&
             LookupLibInfo(*ParentFunc).has(LibFunc_memmove))
           break;
 
         if (expandMemMoveAsLoop(Memmove, TTI)) {
           Changed = true;
           Memmove->eraseFromParent();
         }
       }
 
       break;
     }
     case Intrinsic::memset: {
       auto *Memset = cast<MemSetInst>(Inst);
       Function *ParentFunc = Memset->getFunction();
       const TargetTransformInfo &TTI = LookupTTI(*ParentFunc);
-      if (shouldExpandMemIntrinsicWithSize(Memset->getLength(), TTI)) {
+      if (!IsMinSize &&
+          shouldExpandMemIntrinsicWithSize(Memset->getLength(), TTI)) {
         if (UseMemIntrinsicLibFunc &&
             LookupLibInfo(*Memset->getFunction()).has(LibFunc_memset))
           break;
 
         expandMemSetAsLoop(Memset);
         Changed = true;
         Memset->eraseFromParent();
       }

llvm/test/CodeGen/ARM/no-expand-memcpy-minsize-no-builtins.ll

+; RUN: llc < %s | FileCheck %s
+
+target datalayout = "e-m:o-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "thumbv7em-apple-unknown-macho"
+
+; Function Attrs: nocallback nofree nounwind willreturn memory(argmem: readwrite)
+declare void @llvm.memcpy.p0.p0.i32(ptr noalias nocapture writeonly, ptr noalias nocapture readonly, i32, i1 immarg) #0
+
+; Check we don't expand memcpy to a loop when the caller is a minsize function,
+; even if we have no-builtins attached.
+define arm_aapcs_vfpcc void @test(ptr %p1, ptr %p2) #1 {
+  ; CHECK-LABEL: test:
+  ; CHECK: bl _memcpy
+  call void @llvm.memcpy.p0.p0.i32(ptr %p1, ptr %p2, i32 128, i1 false)
+  ret void
+}
+
+attributes #0 = { nocallback nofree nounwind willreturn memory(argmem: readwrite) }
+attributes #1 = { "no-builtins" minsize }