[PATCH] D37406: [TailCall] Allow llvm.memcpy/memset/memmove to be tail calls when parent function return the intrinsics's first argument

[Friedman, Eli via llvm-commits]

On 9/7/2017 4:26 PM, Wei Mi wrote:
> On Thu, Sep 7, 2017 at 12:34 PM, Eli Friedman via Phabricator
> <reviews at reviews.llvm.org> wrote:
>> efriedma added inline comments.
>>
>>
>> ================
>> Comment at: llvm/trunk/lib/CodeGen/Analysis.cpp:569
>> +  // Intrinsic like llvm.memcpy has no return value, but will return the
>> +  // first argument if it is expanded as libcall.
>> +  const CallInst *Call = cast<CallInst>(I);
>> ----------------
>> This is only true if the argument is specifically expanded to a libcall to the C library function "memcpy".  If the target expands it to some other library call (e.g., __aeabi_memcpy, like we do on Android), it isn't a tail call, and we miscompile.
> Sorry about it. Plan to fix it using TLI.getLibcallName(RTLIB::MEMCPY)
> like below. Does it look ok?
>
> --- lib/CodeGen/Analysis.cpp (revision 309240)
> +++ lib/CodeGen/Analysis.cpp (working copy)
> @@ -565,6 +565,19 @@ bool llvm::returnTypeIsEligibleForTailCa
>       return false;
>
>     const Value *RetVal = Ret->getOperand(0), *CallVal = I;
> +  // Intrinsic like llvm.memcpy has no return value, but will return the
> +  // first argument if it is expanded as libcall.
> +  const CallInst *Call = cast<CallInst>(I);
> +  if (Intrinsic::ID IID = Call->getCalledFunction()->getIntrinsicID())
> +    if (((IID == Intrinsic::memcpy &&
> +          TLI.getLibcallName(RTLIB::MEMCPY) == StringRef("memcpy")) ||
> +         (IID == Intrinsic::memmove &&
> +          TLI.getLibcallName(RTLIB::MEMMOVE) == StringRef("memmove")) ||
> +         (IID == Intrinsic::memset &&
> +          TLI.getLibcallName(RTLIB::MEMSET) == StringRef("memset"))) &&
> +        RetVal == Call->getArgOperand(0))
> +      return true;
> +
>     SmallVector<unsigned, 4> RetPath, CallPath;
>     SmallVector<CompositeType *, 4> RetSubTypes, CallSubTypes;

Yes, that's fine... but please update the comment to note why you're 
writing the check this way.

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project