<div dir="ltr"><div class="gmail_quote">On Wed, May 20, 2015 at 6:52 PM Ahmed Bougacha <<a href="mailto:ahmed.bougacha@gmail.com">ahmed.bougacha@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, May 20, 2015 at 6:18 PM, Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>> wrote:<br>
> On 20 May 2015 at 16:55, Ahmed Bougacha <<a href="mailto:ahmed.bougacha@gmail.com" target="_blank">ahmed.bougacha@gmail.com</a>> wrote:<br>
>><br>
>> Author: ab<br>
>> Date: Wed May 20 18:55:16 2015<br>
>> New Revision: 237858<br>
>><br>
>> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D237858-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=RpRm1hyb_hzO6sXdAuSsckJ8t8TZhHBnkoGhGgphJlQ&s=ltvqDs-JPl2agAEf5YoK4m95OQYol04F-5SMq7508mk&e=" target="_blank">http://llvm.org/viewvc/llvm-project?rev=237858&view=rev</a><br>
>> Log:<br>
>> [MemCpyOpt] Don't move the memset when optimizing memset+memcpy.<br>
>><br>
>> Fixes PR23599, another miscompile introduced by r235232: when there is<br>
>> another dependency on the destination of the created memset (i.e., the<br>
>> part of the original destination that the memcpy doesn't depend on)<br>
>> between the memcpy and the original memset, we would insert the created<br>
>> memset after the memcpy, and thus after the other dependency.<br>
>><br>
>> Instead, insert the created memset right after the old one.<br>
><br>
><br>
> New crash. Testcase:<br>
<br>
Ah, of course.  Let's fall back to the full dependency check: r237874.<br>
<br>
Please have a look!  Clearly I'll never get this right.<br></blockquote><div><br></div><div>If you're not *really sure* that this patch is the only patch needed to make this correct, please revert all of these including the revision that introduced the miscompile. The tree is being left way too unstable for way too long. There comes a point at which it isn't reasonable to keep trying to fix forward and we need to revert to green and take another clean approach to the patch and only after more extensive testing.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
-Ahmed<br>
<br>
> $ cat b.c<br>
> void test(int a, char *b, int c) {<br>
>   memset(b, 0, a);<br>
>   memcpy(b, 0, c);<br>
> }<br>
><br>
> $ clang -cc1 -triple x86_64-pc-linux-gnu -x c b.c -emit-obj -O1 -o /dev/null<br>
> b.c:2:3: warning: implicitly declaring library function 'memset' with type<br>
> 'void *(void *, int, unsigned long)'<br>
>   memset(b, 0, a);<br>
>   ^<br>
> b.c:2:3: note: include the header <string.h> or explicitly provide a<br>
> declaration for 'memset'<br>
> b.c:3:3: warning: implicitly declaring library function 'memcpy' with type<br>
> 'void *(void *, const void *, unsigned long)'<br>
>   memcpy(b, 0, c);<br>
>   ^<br>
> b.c:3:3: note: include the header <string.h> or explicitly provide a<br>
> declaration for 'memcpy'<br>
> Instruction does not dominate all uses!<br>
>   %6 = sext i32 %c to i64<br>
>   %2 = sub nsw i64 %1, %6<br>
> Instruction does not dominate all uses!<br>
>   %6 = sext i32 %c to i64<br>
>   %5 = getelementptr i8, i8* %b, i64 %6<br>
> fatal error: error in backend: Broken function found, compilation aborted!<br>
><br>
>><br>
>> Modified:<br>
>>     llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp<br>
>>     llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-redundant-memset.ll<br>
>><br>
>> Modified: llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp<br>
>> URL:<br>
>> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Transforms_Scalar_MemCpyOptimizer.cpp-3Frev-3D237858-26r1-3D237857-26r2-3D237858-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=RpRm1hyb_hzO6sXdAuSsckJ8t8TZhHBnkoGhGgphJlQ&s=ep5pVYtp2GK2Je8cu6Esys8awQPlynrQxxnlC_iTo0Q&e=" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp?rev=237858&r1=237857&r2=237858&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp (original)<br>
>> +++ llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp Wed May 20<br>
>> 18:55:16 2015<br>
>> @@ -874,7 +874,7 @@ bool MemCpyOpt::processMemSetMemCpyDepen<br>
>>      if (ConstantInt *SrcSizeC = dyn_cast<ConstantInt>(SrcSize))<br>
>>        Align = MinAlign(SrcSizeC->getZExtValue(), DestAlign);<br>
>><br>
>> -  IRBuilder<> Builder(MemCpy->getNextNode());<br>
>> +  IRBuilder<> Builder(MemSet->getNextNode());<br>
>><br>
>>    // If the sizes have different types, zext the smaller one.<br>
>>    if (DestSize->getType() != SrcSize->getType()) {<br>
>><br>
>> Modified:<br>
>> llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-redundant-memset.ll<br>
>> URL:<br>
>> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_Transforms_MemCpyOpt_memset-2Dmemcpy-2Dredundant-2Dmemset.ll-3Frev-3D237858-26r1-3D237857-26r2-3D237858-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=RpRm1hyb_hzO6sXdAuSsckJ8t8TZhHBnkoGhGgphJlQ&s=1LXQq03vOjQVIYCs2eMiAPfbvUXxTMgSh1j1V3vVKOE&e=" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-redundant-memset.ll?rev=237858&r1=237857&r2=237858&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-redundant-memset.ll<br>
>> (original)<br>
>> +++ llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-redundant-memset.ll<br>
>> Wed May 20 18:55:16 2015<br>
>> @@ -3,12 +3,12 @@<br>
>>  target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"<br>
>><br>
>>  ; CHECK-LABEL: define void @test<br>
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,<br>
>> i64 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i64 %src_size<br>
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i64 %dst_size, %src_size<br>
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i64 %dst_size, %src_size<br>
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i64 0, i64<br>
>> [[SIZEDIFF]]<br>
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[DST]], i8 %c, i64<br>
>> [[SIZE]], i32 1, i1 false)<br>
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,<br>
>> i64 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-NEXT: ret void<br>
>>  define void @test(i8* %src, i64 %src_size, i8* %dst, i64 %dst_size, i8<br>
>> %c) {<br>
>>    call void @llvm.memset.p0i8.i64(i8* %dst, i8 %c, i64 %dst_size, i32 1,<br>
>> i1 false)<br>
>> @@ -17,13 +17,13 @@ define void @test(i8* %src, i64 %src_siz<br>
>>  }<br>
>><br>
>>  ; CHECK-LABEL: define void @test_different_types_i32_i64<br>
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,<br>
>> i64 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-DAG: [[DSTSIZE:%[0-9]+]] = zext i32 %dst_size to i64<br>
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i64 %src_size<br>
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i64 [[DSTSIZE]], %src_size<br>
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i64 [[DSTSIZE]], %src_size<br>
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i64 0, i64<br>
>> [[SIZEDIFF]]<br>
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[DST]], i8 %c, i64<br>
>> [[SIZE]], i32 1, i1 false)<br>
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,<br>
>> i64 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-NEXT: ret void<br>
>>  define void @test_different_types_i32_i64(i8* %dst, i8* %src, i32<br>
>> %dst_size, i64 %src_size, i8 %c) {<br>
>>    call void @llvm.memset.p0i8.i32(i8* %dst, i8 %c, i32 %dst_size, i32 1,<br>
>> i1 false)<br>
>> @@ -32,13 +32,13 @@ define void @test_different_types_i32_i6<br>
>>  }<br>
>><br>
>>  ; CHECK-LABEL: define void @test_different_types_i128_i32<br>
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dst, i8* %src,<br>
>> i32 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-DAG: [[SRCSIZE:%[0-9]+]] = zext i32 %src_size to i128<br>
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i128<br>
>> [[SRCSIZE]]<br>
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i128 %dst_size, [[SRCSIZE]]<br>
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i128 %dst_size, [[SRCSIZE]]<br>
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i128 0, i128<br>
>> [[SIZEDIFF]]<br>
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i128(i8* [[DST]], i8 %c, i128<br>
>> [[SIZE]], i32 1, i1 false)<br>
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dst, i8* %src,<br>
>> i32 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-NEXT: ret void<br>
>>  define void @test_different_types_i128_i32(i8* %dst, i8* %src, i128<br>
>> %dst_size, i32 %src_size, i8 %c) {<br>
>>    call void @llvm.memset.p0i8.i128(i8* %dst, i8 %c, i128 %dst_size, i32<br>
>> 1, i1 false)<br>
>> @@ -47,13 +47,13 @@ define void @test_different_types_i128_i<br>
>>  }<br>
>><br>
>>  ; CHECK-LABEL: define void @test_different_types_i32_i128<br>
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i128(i8* %dst, i8* %src,<br>
>> i128 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-DAG: [[DSTSIZE:%[0-9]+]] = zext i32 %dst_size to i128<br>
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i128 %src_size<br>
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i128 [[DSTSIZE]], %src_size<br>
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i128 [[DSTSIZE]], %src_size<br>
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i128 0, i128<br>
>> [[SIZEDIFF]]<br>
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i128(i8* [[DST]], i8 %c, i128<br>
>> [[SIZE]], i32 1, i1 false)<br>
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i128(i8* %dst, i8* %src,<br>
>> i128 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-NEXT: ret void<br>
>>  define void @test_different_types_i32_i128(i8* %dst, i8* %src, i32<br>
>> %dst_size, i128 %src_size, i8 %c) {<br>
>>    call void @llvm.memset.p0i8.i32(i8* %dst, i8 %c, i32 %dst_size, i32 1,<br>
>> i1 false)<br>
>> @@ -62,13 +62,13 @@ define void @test_different_types_i32_i1<br>
>>  }<br>
>><br>
>>  ; CHECK-LABEL: define void @test_different_types_i64_i32<br>
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dst, i8* %src,<br>
>> i32 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-DAG: [[SRCSIZE:%[0-9]+]] = zext i32 %src_size to i64<br>
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i64<br>
>> [[SRCSIZE]]<br>
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i64 %dst_size, [[SRCSIZE]]<br>
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i64 %dst_size, [[SRCSIZE]]<br>
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i64 0, i64<br>
>> [[SIZEDIFF]]<br>
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[DST]], i8 %c, i64<br>
>> [[SIZE]], i32 1, i1 false)<br>
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dst, i8* %src,<br>
>> i32 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-NEXT: ret void<br>
>>  define void @test_different_types_i64_i32(i8* %dst, i8* %src, i64<br>
>> %dst_size, i32 %src_size, i8 %c) {<br>
>>    call void @llvm.memset.p0i8.i64(i8* %dst, i8 %c, i64 %dst_size, i32 1,<br>
>> i1 false)<br>
>> @@ -102,12 +102,12 @@ define void @test_align_memcpy(i8* %src,<br>
>><br>
>>  ; CHECK-LABEL: define void @test_non_i8_dst_type<br>
>>  ; CHECK-NEXT: %dst = bitcast i64* %dst_pi64 to i8*<br>
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,<br>
>> i64 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i64 %src_size<br>
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i64 %dst_size, %src_size<br>
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i64 %dst_size, %src_size<br>
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i64 0, i64<br>
>> [[SIZEDIFF]]<br>
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[DST]], i8 %c, i64<br>
>> [[SIZE]], i32 1, i1 false)<br>
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,<br>
>> i64 %src_size, i32 1, i1 false)<br>
>>  ; CHECK-NEXT: ret void<br>
>>  define void @test_non_i8_dst_type(i8* %src, i64 %src_size, i64*<br>
>> %dst_pi64, i64 %dst_size, i8 %c) {<br>
>>    %dst = bitcast i64* %dst_pi64 to i8*<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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" target="_blank">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>
</blockquote></div></div>