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

[David Wiberg]

2014-07-06 5:07 GMT+02:00 Nick Lewycky <nlewycky at google.com>:
> On 29 June 2014 11:08, David Wiberg <dwiberg at gmail.com> wrote:
>>
>> Ping
>
>
> +  // Check that src isn't captured by the called function since the
> +  // transformation can cause aliasing issues in that case.
> +  for (unsigned i = 0; i < CS.arg_size(); ++i)
>
> LLVM style is to write "for (unsigned i = 0, e = CS.arg_size(); i != e;
> ++i)". See
> http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
>
> +    if (CS.getArgument(i) == cpySrc && !CS.doesNotCapture(i))
> +      return false;
>
> This looks great. I double-checked the changes to the tests to make sure
> that you weren't simply applying nocapture on things that were actually
> capturing, and it looks good to me.
>
> Do you have commit access?

Thanks for the review and the information, I have updated the patch accordingly.

I do not have commit access.

- David

>
>> 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
>> >>
>> >>
>> _______________________________________________
>> 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-4.patch
Type: text/x-patch
Size: 4813 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140707/2ea30d19/attachment.bin>