Fix SROA regression causing data corruption, fixes PR19250

[dotdash]

Summary

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.

[dotdash]

ping

[dotdash]

I'd be grateful if somebody could take the time to review this. The regression fixed by this is blocking rust (the language) from using i1 for its bool type.

Thanks!

[dblaikie]

+Chandler as the Acolyte (wizard? guru? code monkey?) of SROA.

[dotdash]

Added a comment to explain why the whole if-else block has to be moved after
the early continue. Moved the original test into its own file and added new
tests for the slice order indepence of the chosen type for the alloca.

[dotdash]

Sorry, forgot to remove the old @test25 from the original patch.

[dotdash]

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

[dotdash]

Committed in r211082

lib/Transforms/Scalar/SROA.cpp

for (AllocaSlices::const_iterator I = B; I != E; ++I) {
 
     Type *UserTy = nullptr;
     if (LoadInst *LI = dyn_cast<LoadInst>(U->getUser())) {
       UserTy = LI->getType();
     } else if (StoreInst *SI = dyn_cast<StoreInst>(U->getUser())) {
       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
       // entity causing the split. Also skip if the type is not a byte width
       // multiple.
       if (UserITy->getBitWidth() % 8 != 0 ||
           UserITy->getBitWidth() / 8 > (EndOffset - B->beginOffset()))
         continue;
 
       // Track the largest bitwidth integer type used in this way in case there
       // is no common type.
       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;
 }
 
 /// PHI instructions that use an alloca and are subsequently loaded can be
 /// rewritten to load both input pointers in the pred blocks and then PHI the
 /// results, allowing the load of the alloca to be promoted.

test/Transforms/SROA/order_independence.ll

+; 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
+}

test/Transforms/SROA/slicewidth.ll

+; 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
+}