[PATCH] Fix aliasing issue caused by MemCpyOptimizer (PR18304)

[David Wiberg]

Ping

2014-06-23 23:22 GMT+02:00 David Wiberg <dwiberg at gmail.com>:
> 2014-06-23 22:33 GMT+02:00 Nick Lewycky <nicholas at mxc.ca>:
>> David Wiberg wrote:
>>>
>>> 2014-06-21 1:29 GMT+02:00 Philip Reames<listmail at philipreames.com>:
>>>>
>>>> Your change does address a correctness issue and does so in a reasonable
>>>> way.  I wouldn't worry too much about the possible performance
>>>> regression.
>>>>
>>>> One mandatory change:
>>>> - Add a test case which checks the optimization does not apply when
>>>> accessed
>>>> via capturing parameter.
>>>>
>>>> I'd also prefer that you:
>>>> - either use std::distance (rather than iterator subtraction) or
>>>> (preferred)
>>>> use a argument number indexed loop.
>>>> - If you keep the iterator loop, consider a range loop.  Or at least move
>>>> the declaration of B, E into the initializer of the loop to restrict
>>>> their
>>>> scope.
>>>>
>>>> Philip
>>>>
>>>
>>> Thanks for the review. I have added a test case and changed to use an
>>> argument number indexed loop (hopefully) as per your suggestion, see
>>> attached patch.
>>
>>
>> +define void @test() {
>> +  %ptr1 = alloca i8
>> +  %ptr2 = alloca i8
>> +  call void @foo(i8* %ptr2)
>> +  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %ptr1, i8* %ptr2, i32 1, i32 1,
>> i1 false)
>> +  ret void
>> +
>> +  ; Check that the transformation isn't applied if the called function can
>> +  ; capture the pointer argument (i.e. the nocapture attribute isn't
>> present)
>> +  ; CHECK-LABEL: @test(
>> +  ; CHECK: call void @foo(i8* %ptr2)
>> +  ; CHECK-NEXT: call void @llvm.memcpy
>> +}
>>
>> Isn't the transformation legal on this function? If someone wanted to teach
>> memcpyopt to be really smart, it could look at %ptr1 and see that it's
>> entirely dead except for the use in the memcpy, and both %ptr1 and %ptr2 are
>> locally allocated "alloca"s.
>>
>> This testcase could still be added with a "FIXME: missed optz'n" note, but
>> I'm not sure that memcpyopt should even bother with this, it seems like SROA
>> should get it.
>>
>
> Hi Nick,
>
> Thanks for the explanation. I think that you are correct. I was
> looking too much at what the transformation does today. In the
> attached patch the test case is slightly modified to make sure the
> transformation isn't legal.
>
> Since you seemed uncertain about the optimization note I didn't keep
> the previous test.
>
> If approved I need someone to commit it for me.
>
> Best regards
> David
>
>>
>>>> On 06/20/2014 06:13 AM, David Wiberg wrote:
>>>>
>>>> Hello,
>>>>
>>>> The attached patch fixes an aliasing issue caused by MemCpyOptimizer
>>>> by performing the following changes:
>>>> - Added requirement that the callee does not capture the src pointer for
>>>>    the transformation to apply.
>>>> - Updated test cases by adding nocapture attributes where applicable
>>>>
>>>> See http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-June/073819.html
>>>> for some more information. My main concern is performance regressions
>>>> but I haven't been able to get any benchmark results.
>>>>
>>>> Best regards
>>>> David
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>