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

[Björn Steinbrink]

Hi Duncan,

I seem to be unable to get the review system to add you to the list of
mail receivers. Please excuse this extra mail. I had already added the
requested comment and extra test for the order indepence last week, but
happened to re-read the commit message today and rewrote that to be
(hopefully) more correct / easier to understand. I also simplified the
original testcase a bit.

Greetings
Björn

PS: I should probably mention that I will need someone to commit this
for me as I do not have commit access.

On 2014.06.16 22:16:49 +0000, Björn Steinbrink wrote:
> Fix SROA regression causing miscompilation, fixes PR19250
> 
> Summary:
> r199771 accidently changed the logic that excludes integer types that
> are not a byte width multiple from being used as the type used for a
> newly formed partition.
> 
> This means that we can, for example, end up with a partition that spans
> a single byte and has two users, one load with type i1 and another one
> with type i32. The latter is considered to be splittable and expands
> beyond the partition.
> 
> Now in findCommonType, i1 is assigned to Ty before the check that
> rejects i1 because its width is not a byte width multiple. And because
> it is the only type that spans the whole partition, TyIsCommon remains
> true and i1 is returned as the common type.
> 
> Since such types are not actually supported, this causes some bits to be
> zeroed out in the load of the i32 value, because the first byte gets
> truncated to i1 and then zero-extended.
> 
> To fix this problem, we must make sure to check for non-byte width types
> first and always reject them.
> 
> Reviewers: dexonsmith
> Cc: aprantl
> 
> Differential Revision: http://reviews.llvm.org/D3297
> 
> http://reviews.llvm.org/D3297
> 
> Files:
>   lib/Transforms/Scalar/SROA.cpp
>   test/Transforms/SROA/order_independence.ll
>   test/Transforms/SROA/slicewidth.ll

> Index: lib/Transforms/Scalar/SROA.cpp
> ===================================================================
> --- lib/Transforms/Scalar/SROA.cpp
> +++ lib/Transforms/Scalar/SROA.cpp
> @@ -1032,11 +1032,6 @@
>        UserTy = SI->getValueOperand()->getType();
>      }
>  
> -    if (!UserTy || (Ty && Ty != UserTy))
> -      TyIsCommon = false; // Give up on anything but an iN type.
> -    else
> -      Ty = UserTy;
> -
>      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
> @@ -1051,6 +1046,13 @@
>        if (!ITy || ITy->getBitWidth() < UserITy->getBitWidth())
>          ITy = UserITy;
>      }
> +
> +    // To keep thing independent from the order of slices, Ty and TyIsCommon
> +    // must not depend on types skipped above
> +    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/order_independence.ll
> ===================================================================
> --- /dev/null
> +++ test/Transforms/SROA/order_independence.ll
> @@ -0,0 +1,38 @@
> +; RUN: opt < %s -sroa -S | FileCheck %s
> +target datalayout = "e-p:64:64:64-p1:16:16:16-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-n8:16:32:64"
> +
> +declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture, i8* nocapture, i32, i32, i1) nounwind
> +
> +; Check that the chosen type for a split is independent from the order of
> +; slices even in case of types that are skipped because their width is not a
> +; byte width multiple
> +
> +define void @skipped_inttype_first({ i16*, i32 }*) {
> +; CHECK-LABEL: @skipped_inttype_first
> +; CHECK: alloca i8*
> +  %arg = alloca { i16*, i32 }, align 8
> +  %2 = bitcast { i16*, i32 }* %0 to i8*
> +  %3 = bitcast { i16*, i32 }* %arg to i8*
> +  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %3, i8* %2, i32 16, i32 8, i1 false)
> +  %b = getelementptr inbounds { i16*, i32 }* %arg, i64 0, i32 0
> +  %pb0 = bitcast i16** %b to i63*
> +  %b0 = load i63* %pb0
> +  %pb1 = bitcast i16** %b to i8**
> +  %b1 = load i8** %pb1
> +  ret void
> +}
> +
> +define void @skipped_inttype_last({ i16*, i32 }*) {
> +; CHECK-LABEL: @skipped_inttype_last
> +; CHECK: alloca i8*
> +  %arg = alloca { i16*, i32 }, align 8
> +  %2 = bitcast { i16*, i32 }* %0 to i8*
> +  %3 = bitcast { i16*, i32 }* %arg to i8*
> +  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %3, i8* %2, i32 16, i32 8, i1 false)
> +  %b = getelementptr inbounds { i16*, i32 }* %arg, i64 0, i32 0
> +  %pb1 = bitcast i16** %b to i8**
> +  %b1 = load i8** %pb1
> +  %pb0 = bitcast i16** %b to i63*
> +  %b0 = load i63* %pb0
> +  ret void
> +}
> Index: test/Transforms/SROA/slicewidth.ll
> ===================================================================
> --- /dev/null
> +++ test/Transforms/SROA/slicewidth.ll
> @@ -0,0 +1,25 @@
> +; RUN: opt < %s -sroa -S | FileCheck %s
> +target datalayout = "e-p:64:64:64-p1:16:16:16-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-n8:16:32:64"
> +
> +declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture, i8* nocapture, i32, i32, i1) nounwind
> +
> +define void @no_split_on_non_byte_width(i32) {
> +; This tests that allocas are not split into slices that are not byte width multiple
> +  %arg = alloca i32 , align 8
> +  store i32 %0, i32* %arg
> +  br label %load_i32
> +
> +load_i32:
> +; CHECK-LABEL: load_i32:
> +; CHECK-NOT: bitcast {{.*}} to i1
> +; CHECK-NOT: zext i1
> +  %r0 = load i32* %arg
> +  br label %load_i1
> +
> +load_i1:
> +; CHECK-LABEL: load_i1:
> +; CHECK: bitcast {{.*}} to i1
> +  %p1 = bitcast i32* %arg to i1*
> +  %t1 = load i1* %p1
> +  ret void
> +}