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

[Wei Mi via llvm-commits]

Thanks. The fix was committed at https://reviews.llvm.org/rL312799

On Thu, Sep 7, 2017 at 6:13 PM, Friedman, Eli <efriedma at codeaurora.org> wrote:
> 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
>