[llvm] r183584 - Fix an assertion in MemCpyOpt pass.

[Shuxin Yang]

On 6/7/13 8:43 PM, Duncan Sands wrote:
> Hi Shuxin,
>
> On 08/06/13 02:51, Shuxin Yang wrote:
>> On 6/7/13 4:39 PM, Eli Friedman wrote:
>>> On Fri, Jun 7, 2013 at 3:45 PM, Shuxin Yang <shuxin.llvm at gmail.com
>>> <mailto:shuxin.llvm at gmail.com>> wrote:
>>>
>>>     Author: shuxin_yang
>>>     Date: Fri Jun  7 17:45:21 2013
>>>     New Revision: 183584
>>>
>>>     URL: http://llvm.org/viewvc/llvm-project?rev=183584&view=rev
>>>     Log:
>>>       Fix an assertion in MemCpyOpt pass.
>>>
>>>       The MemCpyOpt pass is capable of optimizing:
>>>           callee(&S); copy N bytes from S to D.
>>>         into:
>>>           callee(&D);
>>>     subject to some legality constraints.
>>>
>>>       Assertion is triggered when the compiler tries to evalute
>>>     "sizeof(typeof(D))",
>>>     while D is an opaque-typed, 'sret' formal argument of function 
>>> being compiled.
>>>     i.e. the signature of the func being compiled is something like 
>>> this:
>>>       T caller(...,%opaque* noalias nocapture sret %D, ...)
>>>
>>>       The fix is that when come across such situation, instead of 
>>> calling some
>>>     utility functions to get the size of D's type (which will 
>>> crash), we simply
>>>     assume D has at least N bytes as implified by the copy-instruction.
>>>
>>>     rdar://14073661
>>>
>>>     Modified:
>>>         llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>>>         llvm/trunk/test/Transforms/MemCpyOpt/memcpy.ll
>>>
>>>     Modified: llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>>>     URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp?rev=183584&r1=183583&r2=183584&view=diff
>>> ==============================================================================
>>>     --- llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp (original)
>>>     +++ llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp Fri Jun  7
>>>     17:45:21 2013
>>>     @@ -626,8 +626,10 @@ bool MemCpyOpt::performCallSlotOptzn(Ins
>>>            return false;
>>>
>>>          Type *StructTy = 
>>> cast<PointerType>(A->getType())->getElementType();
>>>     -    uint64_t destSize = TD->getTypeAllocSize(StructTy);
>>>     -
>>>     +    // If StructTy is an opaque type, it should have at least 
>>> <cpyLen> bytes,
>>>     +    // as implified by the copy-instruction.
>>>     +    uint64_t destSize = StructTy->isSized() ?
>>>     +                        TD->getTypeAllocSize(StructTy) : cpyLen;
>>>          if (destSize < srcSize)
>>>            return false;
>>>
>>>
>>> Why is it safe to assume StructTy is at least cpyLen bytes?
>> If not, why is it possible copying cpyLen byte from (part of) src to 
>> Dest?
>
> unfortunately you can't assume that the original memcpy is executed, 
> because the
> call might not return:
>
>   call @func(..., src, ...)
> ^ might throw an exception or exit
>   memcpy(dest, src, ...)
>
> Thus you can't assume that writing cpyLen bytes to dest is 
> automatically safe.
>
>   memcpy(dest, src, ...)
> ^ moving the memcpy before the call requires knowing the write to dest 
> is safe
>   call @func(..., dest, ...)
>
> I think you should just bail out if !StructTy->isSized().
>
> Ciao, Duncan.
>
Yes, you are right.  Thank you for pointing out this tricky case.
I will change the code that way.