[PATCH] Fix SROA regression causing data corruption, fixes PR19250

[Duncan P. N. Exon Smith]

+aprantl

> On 2014-Apr-21, at 03:18, Björn Steinbrink <bsteinbr at gmail.com> wrote:
> 
> dotdash added you to the CC list for the revision "Fix SROA regression causing data corruption, fixes PR19250".
> 
> r199771 accidently broke the logic that makes sure that SROA only splits load
> on byte boundaries. If such a split happens, some bits get lost when
> reassembling loads of wider types, causing data corruption.
> 
> Moving the width check up to reject such splits early fixes the problem.
> 

Thanks for working on this.  A few comments inline.

> 
> http://reviews.llvm.org/D3297
> 
> Files:
>  lib/Transforms/Scalar/SROA.cpp
>  test/Transforms/SROA/basictest.ll
> 
> Index: lib/Transforms/Scalar/SROA.cpp
> ===================================================================
> --- lib/Transforms/Scalar/SROA.cpp
> +++ lib/Transforms/Scalar/SROA.cpp
> @@ -1031,11 +1031,6 @@
>       UserTy = SI->getValueOperand()->getType();
>     }
> 
> -    if (!UserTy || (Ty && Ty != UserTy))
> -      TyIsCommon = false; // Give up on anything but an iN type.

I feel like this check (and assignment to `false`) should still be
*here*, before the early `continue`.  Can you explain why you moved it?

> -    else
> -      Ty = UserTy;

(I understand moving the `else` clause.)

> -
>     if (IntegerType *UserITy = dyn_cast_or_null<IntegerType>(UserTy)) {
>       // If the type is larger than the partition, skip it. We only encounter
>       // this for split integer operations where we want to use the type of the
> @@ -1050,6 +1045,11 @@
>       if (!ITy || ITy->getBitWidth() < UserITy->getBitWidth())
>         ITy = UserITy;
>     }
> +
> +    if (!UserTy || (Ty && Ty != UserTy))
> +      TyIsCommon = false; // Give up on anything but an iN type.
> +    else
> +      Ty = UserTy;
>   }
> 
>   return TyIsCommon ? Ty : ITy;
> Index: test/Transforms/SROA/basictest.ll
> ===================================================================
> --- test/Transforms/SROA/basictest.ll
> +++ test/Transforms/SROA/basictest.ll
> @@ -1440,3 +1440,30 @@
>   ret void
> }
> 
> +define i32 @test25({ i1, i32 }*) {

I think you should create a new file for this testcase.  Name it after
the problem that's being fixed, and give the function a good name too.

> +; This tests that splits only happen on integer types that are byte width multiple
> +; CHECK-LABEL: @test25(
> +; CHECK: case0:
> +; CHECK-NOT: zext i1
> +; CHECK: case1:
> +; CHECK: ret i32
> +entry-block:
> +  %arg = alloca { i1, i32 }, align 8
> +  %1 = bitcast { i1, i32 }* %0 to i8*
> +  %2 = bitcast { i1, i32 }* %arg to i8*
> +  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %2, i8* %1, i32 8, i32 4, i1 false)
> +  %discrp = getelementptr inbounds { i1, i32 }* %arg, i64 0, i32 0
> +  %s = getelementptr inbounds { i1, i32 }* %arg, i64 0, i32 1
> +  %discr = load i1* %discrp, align 1
> +  br i1 %discr, label %case0, label %case1
> +
> +case0:                                     ; preds = %entry-block
> +  %r0 = load i32* %s
> +  ret i32 %r0
> +
> +case1:
> +  %p1 = bitcast i32* %s to i1*
> +  %t1 = load i1* %p1
> +  %r1 = zext i1 %t1 to i32
> +  ret i32 %r1
> +}
> <D3297.1.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits