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

[Nick Lewycky]

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.

>> 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