<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 13 July 2014 10:56, 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>Landed in r212970. Thanks for the patch!</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-07-07 18:01 GMT+02:00 David Wiberg <<a href="mailto:dwiberg@gmail.com">dwiberg@gmail.com</a>>:<br>
<div class=""><div class="h5">> 2014-07-06 5:07 GMT+02:00 Nick Lewycky <<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>>:<br>
>> On 29 June 2014 11:08, David Wiberg <<a href="mailto:dwiberg@gmail.com">dwiberg@gmail.com</a>> wrote:<br>
>>><br>
>>> Ping<br>
>><br>
>><br>
>> + // Check that src isn't captured by the called function since the<br>
>> + // transformation can cause aliasing issues in that case.<br>
>> + for (unsigned i = 0; i < CS.arg_size(); ++i)<br>
>><br>
>> LLVM style is to write "for (unsigned i = 0, e = CS.arg_size(); i != e;<br>
>> ++i)". See<br>
>> <a href="http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop" target="_blank">http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop</a><br>
>><br>
>> + if (CS.getArgument(i) == cpySrc && !CS.doesNotCapture(i))<br>
>> + return false;<br>
>><br>
>> This looks great. I double-checked the changes to the tests to make sure<br>
>> that you weren't simply applying nocapture on things that were actually<br>
>> capturing, and it looks good to me.<br>
>><br>
>> Do you have commit access?<br>
><br>
> Thanks for the review and the information, I have updated the patch accordingly.<br>
><br>
> I do not have commit access.<br>
><br>
> - David<br>
><br>
>><br>
>>> 2014-06-23 23:22 GMT+02:00 David Wiberg <<a href="mailto:dwiberg@gmail.com">dwiberg@gmail.com</a>>:<br>
>>> > 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<br>
>>> >>>> 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<br>
>>> >>>> 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,<br>
>>> >> i32 1,<br>
>>> >> i1 false)<br>
>>> >> + ret void<br>
>>> >> +<br>
>>> >> + ; Check that the transformation isn't applied if the called function<br>
>>> >> 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<br>
>>> >> 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<br>
>>> >> %ptr2 are<br>
>>> >> locally allocated "alloca"s.<br>
>>> >><br>
>>> >> This testcase could still be added with a "FIXME: missed optz'n" note,<br>
>>> >> but<br>
>>> >> I'm not sure that memcpyopt should even bother with this, it seems like<br>
>>> >> 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<br>
>>> >>>> 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><br></div></div>