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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

In D155790#4526472 <https://reviews.llvm.org/D155790#4526472>, @efriedma wrote:

> As far as I know, before 3c848194f28decca41b7362f9dd35d4939797724 <https://reviews.llvm.org/rG3c848194f28decca41b7362f9dd35d4939797724>, 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 <https://reviews.llvm.org/rG3c848194f28decca41b7362f9dd35d4939797724> changes things so that instead of calling "memcpy", we generate a bunch of terrible inline code.  This apparently landed without review?



In D155790#4528021 <https://reviews.llvm.org/D155790#4528021>, @arsenm wrote:

> In D155790#4526472 <https://reviews.llvm.org/D155790#4526472>, @efriedma wrote:
>
>> As far as I know, before 3c848194f28decca41b7362f9dd35d4939797724 <https://reviews.llvm.org/rG3c848194f28decca41b7362f9dd35d4939797724>, 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 <https://reviews.llvm.org/rG3c848194f28decca41b7362f9dd35d4939797724> changes things so that instead of calling "memcpy", we generate a bunch of terrible inline code.
>
>
>
>> This apparently landed without review?
>
> This was D152383 <https://reviews.llvm.org/D152383>


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155790