[llvm] r237858 - [MemCpyOpt] Don't move the memset when optimizing memset+memcpy.

Ahmed Bougacha ahmed.bougacha at gmail.com
Wed May 20 18:49:07 PDT 2015 

On Wed, May 20, 2015 at 6:18 PM, Nick Lewycky <nlewycky at google.com> wrote:
> On 20 May 2015 at 16:55, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
>>
>> Author: ab
>> Date: Wed May 20 18:55:16 2015
>> New Revision: 237858
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=237858&view=rev
>> Log:
>> [MemCpyOpt] Don't move the memset when optimizing memset+memcpy.
>>
>> Fixes PR23599, another miscompile introduced by r235232: when there is
>> another dependency on the destination of the created memset (i.e., the
>> part of the original destination that the memcpy doesn't depend on)
>> between the memcpy and the original memset, we would insert the created
>> memset after the memcpy, and thus after the other dependency.
>>
>> Instead, insert the created memset right after the old one.
>
>
> New crash. Testcase:

Ah, of course.  Let's fall back to the full dependency check: r237874.

Please have a look!  Clearly I'll never get this right.

-Ahmed

> $ cat b.c
> void test(int a, char *b, int c) {
>   memset(b, 0, a);
>   memcpy(b, 0, c);
> }
>
> $ clang -cc1 -triple x86_64-pc-linux-gnu -x c b.c -emit-obj -O1 -o /dev/null
> b.c:2:3: warning: implicitly declaring library function 'memset' with type
> 'void *(void *, int, unsigned long)'
>   memset(b, 0, a);
>   ^
> b.c:2:3: note: include the header <string.h> or explicitly provide a
> declaration for 'memset'
> b.c:3:3: warning: implicitly declaring library function 'memcpy' with type
> 'void *(void *, const void *, unsigned long)'
>   memcpy(b, 0, c);
>   ^
> b.c:3:3: note: include the header <string.h> or explicitly provide a
> declaration for 'memcpy'
> Instruction does not dominate all uses!
>   %6 = sext i32 %c to i64
>   %2 = sub nsw i64 %1, %6
> Instruction does not dominate all uses!
>   %6 = sext i32 %c to i64
>   %5 = getelementptr i8, i8* %b, i64 %6
> fatal error: error in backend: Broken function found, compilation aborted!
>
>>
>> Modified:
>>     llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>>     llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-redundant-memset.ll
>>
>> Modified: llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp?rev=237858&r1=237857&r2=237858&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp Wed May 20
>> 18:55:16 2015
>> @@ -874,7 +874,7 @@ bool MemCpyOpt::processMemSetMemCpyDepen
>>      if (ConstantInt *SrcSizeC = dyn_cast<ConstantInt>(SrcSize))
>>        Align = MinAlign(SrcSizeC->getZExtValue(), DestAlign);
>>
>> -  IRBuilder<> Builder(MemCpy->getNextNode());
>> +  IRBuilder<> Builder(MemSet->getNextNode());
>>
>>    // If the sizes have different types, zext the smaller one.
>>    if (DestSize->getType() != SrcSize->getType()) {
>>
>> Modified:
>> llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-redundant-memset.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-redundant-memset.ll?rev=237858&r1=237857&r2=237858&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-redundant-memset.ll
>> (original)
>> +++ llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-redundant-memset.ll
>> Wed May 20 18:55:16 2015
>> @@ -3,12 +3,12 @@
>>  target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
>>
>>  ; CHECK-LABEL: define void @test
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,
>> i64 %src_size, i32 1, i1 false)
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i64 %src_size
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i64 %dst_size, %src_size
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i64 %dst_size, %src_size
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i64 0, i64
>> [[SIZEDIFF]]
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[DST]], i8 %c, i64
>> [[SIZE]], i32 1, i1 false)
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,
>> i64 %src_size, i32 1, i1 false)
>>  ; CHECK-NEXT: ret void
>>  define void @test(i8* %src, i64 %src_size, i8* %dst, i64 %dst_size, i8
>> %c) {
>>    call void @llvm.memset.p0i8.i64(i8* %dst, i8 %c, i64 %dst_size, i32 1,
>> i1 false)
>> @@ -17,13 +17,13 @@ define void @test(i8* %src, i64 %src_siz
>>  }
>>
>>  ; CHECK-LABEL: define void @test_different_types_i32_i64
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,
>> i64 %src_size, i32 1, i1 false)
>>  ; CHECK-DAG: [[DSTSIZE:%[0-9]+]] = zext i32 %dst_size to i64
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i64 %src_size
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i64 [[DSTSIZE]], %src_size
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i64 [[DSTSIZE]], %src_size
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i64 0, i64
>> [[SIZEDIFF]]
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[DST]], i8 %c, i64
>> [[SIZE]], i32 1, i1 false)
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,
>> i64 %src_size, i32 1, i1 false)
>>  ; CHECK-NEXT: ret void
>>  define void @test_different_types_i32_i64(i8* %dst, i8* %src, i32
>> %dst_size, i64 %src_size, i8 %c) {
>>    call void @llvm.memset.p0i8.i32(i8* %dst, i8 %c, i32 %dst_size, i32 1,
>> i1 false)
>> @@ -32,13 +32,13 @@ define void @test_different_types_i32_i6
>>  }
>>
>>  ; CHECK-LABEL: define void @test_different_types_i128_i32
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dst, i8* %src,
>> i32 %src_size, i32 1, i1 false)
>>  ; CHECK-DAG: [[SRCSIZE:%[0-9]+]] = zext i32 %src_size to i128
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i128
>> [[SRCSIZE]]
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i128 %dst_size, [[SRCSIZE]]
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i128 %dst_size, [[SRCSIZE]]
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i128 0, i128
>> [[SIZEDIFF]]
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i128(i8* [[DST]], i8 %c, i128
>> [[SIZE]], i32 1, i1 false)
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dst, i8* %src,
>> i32 %src_size, i32 1, i1 false)
>>  ; CHECK-NEXT: ret void
>>  define void @test_different_types_i128_i32(i8* %dst, i8* %src, i128
>> %dst_size, i32 %src_size, i8 %c) {
>>    call void @llvm.memset.p0i8.i128(i8* %dst, i8 %c, i128 %dst_size, i32
>> 1, i1 false)
>> @@ -47,13 +47,13 @@ define void @test_different_types_i128_i
>>  }
>>
>>  ; CHECK-LABEL: define void @test_different_types_i32_i128
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i128(i8* %dst, i8* %src,
>> i128 %src_size, i32 1, i1 false)
>>  ; CHECK-DAG: [[DSTSIZE:%[0-9]+]] = zext i32 %dst_size to i128
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i128 %src_size
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i128 [[DSTSIZE]], %src_size
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i128 [[DSTSIZE]], %src_size
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i128 0, i128
>> [[SIZEDIFF]]
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i128(i8* [[DST]], i8 %c, i128
>> [[SIZE]], i32 1, i1 false)
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i128(i8* %dst, i8* %src,
>> i128 %src_size, i32 1, i1 false)
>>  ; CHECK-NEXT: ret void
>>  define void @test_different_types_i32_i128(i8* %dst, i8* %src, i32
>> %dst_size, i128 %src_size, i8 %c) {
>>    call void @llvm.memset.p0i8.i32(i8* %dst, i8 %c, i32 %dst_size, i32 1,
>> i1 false)
>> @@ -62,13 +62,13 @@ define void @test_different_types_i32_i1
>>  }
>>
>>  ; CHECK-LABEL: define void @test_different_types_i64_i32
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dst, i8* %src,
>> i32 %src_size, i32 1, i1 false)
>>  ; CHECK-DAG: [[SRCSIZE:%[0-9]+]] = zext i32 %src_size to i64
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i64
>> [[SRCSIZE]]
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i64 %dst_size, [[SRCSIZE]]
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i64 %dst_size, [[SRCSIZE]]
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i64 0, i64
>> [[SIZEDIFF]]
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[DST]], i8 %c, i64
>> [[SIZE]], i32 1, i1 false)
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dst, i8* %src,
>> i32 %src_size, i32 1, i1 false)
>>  ; CHECK-NEXT: ret void
>>  define void @test_different_types_i64_i32(i8* %dst, i8* %src, i64
>> %dst_size, i32 %src_size, i8 %c) {
>>    call void @llvm.memset.p0i8.i64(i8* %dst, i8 %c, i64 %dst_size, i32 1,
>> i1 false)
>> @@ -102,12 +102,12 @@ define void @test_align_memcpy(i8* %src,
>>
>>  ; CHECK-LABEL: define void @test_non_i8_dst_type
>>  ; CHECK-NEXT: %dst = bitcast i64* %dst_pi64 to i8*
>> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,
>> i64 %src_size, i32 1, i1 false)
>>  ; CHECK-DAG: [[DST:%[0-9]+]] = getelementptr i8, i8* %dst, i64 %src_size
>>  ; CHECK-DAG: [[ULE:%[0-9]+]] = icmp ule i64 %dst_size, %src_size
>>  ; CHECK-DAG: [[SIZEDIFF:%[0-9]+]] = sub i64 %dst_size, %src_size
>>  ; CHECK-DAG: [[SIZE:%[0-9]+]] = select i1 [[ULE]], i64 0, i64
>> [[SIZEDIFF]]
>>  ; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[DST]], i8 %c, i64
>> [[SIZE]], i32 1, i1 false)
>> +; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src,
>> i64 %src_size, i32 1, i1 false)
>>  ; CHECK-NEXT: ret void
>>  define void @test_non_i8_dst_type(i8* %src, i64 %src_size, i64*
>> %dst_pi64, i64 %dst_size, i8 %c) {
>>    %dst = bitcast i64* %dst_pi64 to i8*
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>

More information about the llvm-commits mailing list