[SROA] Avoid splitting loads/stores with irregular type

[LemonBoy]

Summary

Upon encountering loads/stores on types whose size is not a multiple of 8 bits the SROA pass would either trip an assertion or use logic that was not meant to work with such irregularly-sized types.

[LemonBoy]

Run clang-format on the modified line.

[aeubanks]

lgtm

[thakis]

The new test is failing at least on linux and mac:

• https://lab.llvm.org/buildbot/#/builders/109/builds/16382
• http://45.33.8.238/macm1/11066/step_11.txt
• most bots on https://lab.llvm.org/buildbot/#/console are red

I reverted this in 205cde63c70e017a71d1ec06377421f7733f2ad5 for now.

[LemonBoy]

Sorry for the noise, the test got stale since I first uploaded it. I'll push again once this is sorted out.

[marler8997]

Looks like @LemonBoy updated the test. I'm hitting this issue so wanted to get the status of this patch to know if I should include it?

[aeubanks]

I think this was already relanded. Do you have a repro?

[marler8997]

In D99435#2961594, @aeubanks wrote:

I think this was already relanded. Do you have a repro?

Perfect it looks like you're right. I just read the messages in the thread and saw it was reverted but didn't see that it was merged after that, but the current state of the repo does appear to have this change. Great news thanks.

llvm/lib/Transforms/Scalar/SROA.cpp

void visitGetElementPtrInst(GetElementPtrInst &GEPI) {
     return Base::visitGetElementPtrInst(GEPI);
   }
 
   void handleLoadOrStore(Type *Ty, Instruction &I, const APInt &Offset,
                          uint64_t Size, bool IsVolatile) {
     // We allow splitting of non-volatile loads and stores where the type is an
     // integer type. These may be used to implement 'memcpy' or other "transfer
     // of bits" patterns.
-    bool IsSplittable = Ty->isIntegerTy() && !IsVolatile;
+    bool IsSplittable =
+        Ty->isIntegerTy() && !IsVolatile && DL.typeSizeEqualsStoreSize(Ty);
 
     insertUse(I, Offset, Size, IsSplittable);
   }
 
   void visitLoadInst(LoadInst &LI) {
     assert((!LI.isSimple() || LI.getType()->isSingleValueType()) &&
            "All simple FCA loads should have been pre-split");
 
bool SROA::presplitLoadsAndStores(AllocaInst &AI, AllocaSlices &AS) {
   // them to the alloca slices.
   SmallDenseMap<LoadInst *, std::vector<LoadInst *>, 1> SplitLoadsMap;
   std::vector<LoadInst *> SplitLoads;
   const DataLayout &DL = AI.getModule()->getDataLayout();
   for (LoadInst *LI : Loads) {
     SplitLoads.clear();
 
     IntegerType *Ty = cast<IntegerType>(LI->getType());
+    assert(Ty->getBitWidth() % 8 == 0);
     uint64_t LoadSize = Ty->getBitWidth() / 8;
     assert(LoadSize > 0 && "Cannot have a zero-sized integer load!");
 
     auto &Offsets = SplitOffsetsMap[LI];
     assert(LoadSize == Offsets.S->endOffset() - Offsets.S->beginOffset() &&
            "Slice size should always match load size exactly!");
     uint64_t BaseOffset = Offsets.S->beginOffset();
     assert(BaseOffset + LoadSize > BaseOffset &&
bool SROA::presplitLoadsAndStores(AllocaInst &AI, AllocaSlices &AS) {
   // Second, we rewrite all of the split stores. At this point, we know that
   // all loads from this alloca have been split already. For stores of such
   // loads, we can simply look up the pre-existing split loads. For stores of
   // other loads, we split those loads first and then write split stores of
   // them.
   for (StoreInst *SI : Stores) {
     auto *LI = cast<LoadInst>(SI->getValueOperand());
     IntegerType *Ty = cast<IntegerType>(LI->getType());
+    assert(Ty->getBitWidth() % 8 == 0);
     uint64_t StoreSize = Ty->getBitWidth() / 8;
     assert(StoreSize > 0 && "Cannot have a zero-sized integer store!");
 
     auto &Offsets = SplitOffsetsMap[SI];
     assert(StoreSize == Offsets.S->endOffset() - Offsets.S->beginOffset() &&
            "Slice size should always match load size exactly!");
     uint64_t BaseOffset = Offsets.S->beginOffset();
     assert(BaseOffset + StoreSize > BaseOffset &&

llvm/test/Transforms/SROA/irregular-type.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -sroa -S | FileCheck %s
+
+%S = type { [4 x i8] }
+
+; Ensure the load/store of integer types whose size is not equal to the store
+; size are not split.
+
+define i8 @foo(i23 %0) {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  Entry:
+; CHECK-NEXT:    [[DOTSROA_0:%.*]] = alloca [3 x i8], align 8
+; CHECK-NEXT:    [[DOTSROA_0_0__SROA_CAST1:%.*]] = bitcast [3 x i8]* [[DOTSROA_0]] to i23*
+; CHECK-NEXT:    store i23 [[TMP0:%.*]], i23* [[DOTSROA_0_0__SROA_CAST1]], align 8
+; CHECK-NEXT:    [[DOTSROA_0_1__SROA_IDX2:%.*]] = getelementptr inbounds [3 x i8], [3 x i8]* [[DOTSROA_0]], i64 0, i64 1
+; CHECK-NEXT:    [[DOTSROA_0_1__SROA_0_1_:%.*]] = load i8, i8* [[DOTSROA_0_1__SROA_IDX2]], align 1
+; CHECK-NEXT:    ret i8 [[DOTSROA_0_1__SROA_0_1_]]
+;
+Entry:
+  %1 = alloca %S
+  %2 = bitcast %S* %1 to i23*
+  store i23 %0, i23* %2
+  %3 = getelementptr inbounds %S, %S* %1, i64 0, i32 0, i32 1
+  %4 = load i8, i8* %3
+  ret i8 %4
+}
+
+define i32 @bar(i16 %0) {
+; CHECK-LABEL: @bar(
+; CHECK-NEXT:  Entry:
+; CHECK-NEXT:    [[DOTSROA_0:%.*]] = alloca [3 x i8], align 8
+; CHECK-NEXT:    [[DOTSROA_0_0__SROA_CAST2:%.*]] = bitcast [3 x i8]* [[DOTSROA_0]] to i16*
+; CHECK-NEXT:    store i16 [[TMP0:%.*]], i16* [[DOTSROA_0_0__SROA_CAST2]], align 8
+; CHECK-NEXT:    [[DOTSROA_0_0_Q_SROA_CAST1:%.*]] = bitcast [3 x i8]* [[DOTSROA_0]] to i17*
+; CHECK-NEXT:    [[DOTSROA_0_0__SROA_0_0_:%.*]] = load i17, i17* [[DOTSROA_0_0_Q_SROA_CAST1]], align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = zext i17 [[DOTSROA_0_0__SROA_0_0_]] to i32
+; CHECK-NEXT:    ret i32 [[TMP1]]
+;
+Entry:
+  %1 = alloca %S
+  %2 = bitcast %S* %1 to i16*
+  store i16 %0, i16* %2
+  %3 = getelementptr inbounds %S, %S* %1, i64 0, i32 0
+  %q = bitcast [4 x i8]* %3 to i17*
+  %4 = load i17, i17* %q
+  %5 = zext i17 %4 to i32
+  ret i32 %5
+}