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