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

[David Wiberg]

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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr18304-3.patch
Type: application/octet-stream
Size: 4805 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140623/85d688e5/attachment.obj>