[SROA] pr37267: fix assertion failure while integer widening

[inouehrs]

Summary

The current integer widening does not support rewriting partial split spice in rewriteIntegerStore (and rewriteIntegerLoad).
This patch adds explicit check for this in isIntegerWideningViableForSlice.
Before r322533, splitting is allowed only for the whole-alloca slice and hence the above case is implicitly rejected by another check if (DL.getTypeStoreSize(ValueTy) > Size) because whole-alloca slice is larger than the partition.

By adding this case, there was no visible difference in the number of widening happened during the bootstrap test.

[uabelho]

This indeed solves the crash I saw. Thanks!

[efriedma]

lib/Transforms/Scalar/SROA.cpp

1994 ↗

(On Diff #146306)

Why does this check belong here, as opposed to the beginning of the function or somewhere in isIntegerWideningViable()?

test/Transforms/SROA/pr37267.ll

6 ↗

(On Diff #146306)

Please add FileCheck lines to show how we split the alloca. A test like this doesn't really make it clear what you're expecting to happen instead of crashing.

[inouehrs]

• modified test case

[inouehrs]

lib/Transforms/Scalar/SROA.cpp

1994 ↗

(On Diff #146306)

visitMemTransferInst seems to be able to handle split tails of memcpy (e.g. when replacing a store to slice 1 in the test case with memcpy). So I do not want to reduce the opportunities by checking this at the beginning of isIntegerWideningViableForSlice.

test/Transforms/SROA/pr37267.ll

6 ↗

(On Diff #146306)

I modified test case with FileCheck lines. Also I add comments to make the behavior clearer.

[efriedma]

Your testcase doesn't actually crash on trunk, unlike the testcase from the bug.

lib/Transforms/Scalar/SROA.cpp

1994 ↗

(On Diff #146306)

So the limitation isn't that the transform is impossible, just that rewriteIntegerStore doesn't know how to insert the right shift? If that's correct, could you state that in the comment a little more explicitly? If not, could you explain a bit more why this isn't possible?

The testcase from the bug specifically hits the store case; can you construct a testcase which hits the load case?

[inouehrs]

• added a test case

[inouehrs]

• comments updated

[inouehrs]

Your testcase doesn't actually crash on trunk, unlike the testcase from the bug.

I fixed the test case. Thank you for pointing this out.

lib/Transforms/Scalar/SROA.cpp

1994 ↗

(On Diff #146306)

I added the second test case which causes an assertion failure while rewriting a load.

[efriedma]

LGTM with one minor comment

lib/Transforms/Scalar/SROA.cpp

1953 ↗

(On Diff #147040)

Instead of doing this implicit signed-unsigned conversion, could you explicitly check "S.beginOffset() < AllocBeginOffset"? It's confusing to have to worry about overflow. (Granted, I don't think the overflowing case is likely to show up in practice, but there isn't anything preventing it.)

llvm/trunk/lib/Transforms/Scalar/SROA.cpp

static bool isIntegerWideningViableForSlice(const Slice &S,
   Use *U = S.getUse();
 
   if (LoadInst *LI = dyn_cast<LoadInst>(U->getUser())) {
     if (LI->isVolatile())
       return false;
     // We can't handle loads that extend past the allocated memory.
     if (DL.getTypeStoreSize(LI->getType()) > Size)
       return false;
+    // So far, AllocaSliceRewriter does not support widening split slice tails
+    // in rewriteIntegerLoad.
+    if (S.beginOffset() < AllocBeginOffset)
+      return false;
     // Note that we don't count vector loads or stores as whole-alloca
     // operations which enable integer widening because we would prefer to use
     // vector widening instead.
     if (!isa<VectorType>(LI->getType()) && RelBegin == 0 && RelEnd == Size)
       WholeAllocaOp = true;
     if (IntegerType *ITy = dyn_cast<IntegerType>(LI->getType())) {
       if (ITy->getBitWidth() < DL.getTypeStoreSizeInBits(ITy))
         return false;
     } else if (RelBegin != 0 || RelEnd != Size ||
                !canConvertValue(DL, AllocaTy, LI->getType())) {
       // Non-integer loads need to be convertible from the alloca type so that
       // they are promotable.
       return false;
     }
   } else if (StoreInst *SI = dyn_cast<StoreInst>(U->getUser())) {
     Type *ValueTy = SI->getValueOperand()->getType();
     if (SI->isVolatile())
       return false;
     // We can't handle stores that extend past the allocated memory.
     if (DL.getTypeStoreSize(ValueTy) > Size)
       return false;
+    // So far, AllocaSliceRewriter does not support widening split slice tails
+    // in rewriteIntegerStore.
+    if (S.beginOffset() < AllocBeginOffset)
+      return false;
     // Note that we don't count vector loads or stores as whole-alloca
     // operations which enable integer widening because we would prefer to use
     // vector widening instead.
     if (!isa<VectorType>(ValueTy) && RelBegin == 0 && RelEnd == Size)
       WholeAllocaOp = true;
     if (IntegerType *ITy = dyn_cast<IntegerType>(ValueTy)) {
       if (ITy->getBitWidth() < DL.getTypeStoreSizeInBits(ITy))
         return false;

llvm/trunk/test/Transforms/SROA/pr37267.ll

+; RUN: opt < %s -sroa -S | FileCheck %s
+target datalayout = "E-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-n32:64-S128"
+target triple = "sparcv9-sun-solaris"
+
+; PR37267
+; Check that we don't crash on this test.
+
+define i16 @f1() {
+; CHECK-LABEL: @f1
+; CHECK: %[[retval:.*]] = add i16 2, 2
+; CHECK: ret i16 %[[retval]]
+
+bb1:
+; This 12-byte alloca is split into partitions as [0,2), [2,4), [4,8), [8,10), [10, 12).
+; The reported error happened when rewriteIntegerStore try to widen a split tail of slice 1 for [4, 8) partition.
+; alloca  012345678901
+; slice 1:  WWWW
+; slice 2:        WWWW
+; slice 3:        RR
+; slice 4:  RR
+
+  %a.3 = alloca [6 x i16], align 1
+; slice 1: [2,6)
+  %_tmp3 = getelementptr inbounds [6 x i16], [6 x i16]* %a.3, i16 0, i16 1
+  %_tmp5 = bitcast i16* %_tmp3 to i32*
+  store i32 131074, i32* %_tmp5, align 1
+; slice 2: [8,12)
+  %_tmp8 = getelementptr inbounds [6 x i16], [6 x i16]* %a.3, i16 0, i16 4
+  %_tmp10 = bitcast i16* %_tmp8 to i32*
+  store i32 131074, i32* %_tmp10, align 1
+; slice 3: [8,10)
+  %_tmp12 = getelementptr inbounds [6 x i16], [6 x i16]* %a.3, i16 0, i16 4
+  %_tmp13 = load i16, i16* %_tmp12, align 1
+; slice 4: [2,4)
+  %_tmp15 = getelementptr inbounds [6 x i16], [6 x i16]* %a.3, i16 0, i16 1
+  %_tmp16 = load i16, i16* %_tmp15, align 1
+
+  %rc = add i16 %_tmp13, %_tmp16
+  ret i16 %rc
+}
+
+define i16 @f2() {
+; CHECK-LABEL: @f2
+; CHECK: %[[retval:.*]] = add i16 2, undef
+; CHECK: ret i16 %[[retval]]
+
+bb1:
+; This 12-byte alloca is split into partitions as [0,2), [2,4), [4,8), [8,10), [10, 12).
+; The reported error happened when visitLoadInst rewrites a split tail of slice 1 for [4, 8) partition.
+; alloca  012345678901
+; slice 1:  RRRR
+; slice 2:        WWWW
+; slice 3:        RR
+; slice 4:  RR
+
+  %a.3 = alloca [6 x i16], align 1
+; slice 1: [2,6)
+  %_tmp3 = getelementptr inbounds [6 x i16], [6 x i16]* %a.3, i16 0, i16 1
+  %_tmp5 = bitcast i16* %_tmp3 to i32*
+  %_tmp6 = load i32, i32* %_tmp5, align 1
+; slice 2: [8,12)
+  %_tmp8 = getelementptr inbounds [6 x i16], [6 x i16]* %a.3, i16 0, i16 4
+  %_tmp10 = bitcast i16* %_tmp8 to i32*
+  store i32 131074, i32* %_tmp10, align 1
+; slice 3: [8,10)
+  %_tmp12 = getelementptr inbounds [6 x i16], [6 x i16]* %a.3, i16 0, i16 4
+  %_tmp13 = load i16, i16* %_tmp12, align 1
+; slice 4: [2,4)
+  %_tmp15 = getelementptr inbounds [6 x i16], [6 x i16]* %a.3, i16 0, i16 1
+  %_tmp16 = load i16, i16* %_tmp15, align 1
+
+  %rc = add i16 %_tmp13, %_tmp16
+  ret i16 %rc
+}