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

Chandler Carruth chandlerc at google.com
Wed May 20 18:57:06 PDT 2015 

On Wed, May 20, 2015 at 6:52 PM Ahmed Bougacha <ahmed.bougacha at gmail.com>
wrote:

> 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.
>

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.


>
> -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
> >
> >
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150521/7c3c1e12/attachment.html>

More information about the llvm-commits mailing list