Fix load alignement when unpacking aggregates structs

[deadalnix]

Summary

Store and loads unpacked by instcombine do not always have the right alignement. This explicitely compute the alignement and set it.

[majnemer]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

977–978 ↗

(On Diff #48150)

Would MinAlign from MathExtras work here? I'm thinking MinAlign(SL->getElementOffset(i), Align).

[deadalnix]

Use MinAlign

[majnemer]

LGTM

[majnemer]

Actually, some last minute questions came up...

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

548 ↗

(On Diff #48158)

I think it'd be more obvious if you used DL.getABITypeAlignment(ST) here seeing as how the langref defines the load's alignment using the language: A value of 0 or an omitted align argument means that the operation has the ABI alignment for the target.

[deadalnix]

Use getABITypeAlignment

[majnemer]

LGTM

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

975–976 ↗

(On Diff #48218)

Is this clang-formatted?

[yuyichao]

Looks like http://reviews.llvm.org/D17158 is backported to 3.8.0 and is causing a segfault. Could this one be backported too?

llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

static Instruction *unpackLoadToAggregate(InstCombiner &IC, LoadInst &LI) {
   // stores here but it isn't clear that this is important.
   if (!LI.isSimple())
     return nullptr;
 
   Type *T = LI.getType();
   if (!T->isAggregateType())
     return nullptr;
 
+  auto Name = LI.getName();
   assert(LI.getAlignment() && "Alignment must be set at this point");
 
   if (auto *ST = dyn_cast<StructType>(T)) {
     // If the struct only have one element, we unpack.
-    unsigned Count = ST->getNumElements();
-    if (Count == 1) {
+    auto NumElements = ST->getNumElements();
+    if (NumElements == 1) {
       LoadInst *NewLoad = combineLoadToNewType(IC, LI, ST->getTypeAtIndex(0U),
                                                ".unpack");
       return IC.replaceInstUsesWith(LI, IC.Builder->CreateInsertValue(
-        UndefValue::get(T), NewLoad, 0, LI.getName()));
+        UndefValue::get(T), NewLoad, 0, Name));
     }
 
     // We don't want to break loads with padding here as we'd loose
     // the knowledge that padding exists for the rest of the pipeline.
     const DataLayout &DL = IC.getDataLayout();
     auto *SL = DL.getStructLayout(ST);
     if (SL->hasPadding())
       return nullptr;
 
-    auto Name = LI.getName();
+    auto Align = LI.getAlignment();
+    if (!Align)
+      Align = DL.getABITypeAlignment(ST);
+
     SmallString<16> LoadName = Name;
     LoadName += ".unpack";
     SmallString<16> EltName = Name;
     EltName += ".elt";
+
     auto *Addr = LI.getPointerOperand();
-    Value *V = UndefValue::get(T);
-    auto *IdxType = Type::getInt32Ty(ST->getContext());
+    auto *IdxType = Type::getInt32Ty(T->getContext());
     auto *Zero = ConstantInt::get(IdxType, 0);
-    for (unsigned i = 0; i < Count; i++) {
+
+    Value *V = UndefValue::get(T);
+    for (unsigned i = 0; i < NumElements; i++) {
       Value *Indices[2] = {
         Zero,
         ConstantInt::get(IdxType, i),
       };
-      auto *Ptr = IC.Builder->CreateInBoundsGEP(ST, Addr, makeArrayRef(Indices), EltName);
-      auto *L = IC.Builder->CreateAlignedLoad(Ptr, LI.getAlignment(),
-                                              LoadName);
+      auto *Ptr = IC.Builder->CreateInBoundsGEP(ST, Addr,
+                                            makeArrayRef(Indices), EltName);
+      auto EltAlign = MinAlign(Align, SL->getElementOffset(i));
+      auto *L = IC.Builder->CreateAlignedLoad(Ptr, EltAlign, LoadName);
       V = IC.Builder->CreateInsertValue(V, L, i);
     }
 
     V->setName(Name);
     return IC.replaceInstUsesWith(LI, V);
   }
 
   if (auto *AT = dyn_cast<ArrayType>(T)) {
if (auto *ST = dyn_cast<StructType>(T)) {
 
     // We don't want to break loads with padding here as we'd loose
     // the knowledge that padding exists for the rest of the pipeline.
     const DataLayout &DL = IC.getDataLayout();
     auto *SL = DL.getStructLayout(ST);
     if (SL->hasPadding())
       return false;
 
+    auto Align = SI.getAlignment();
+    if (!Align)
+      Align = DL.getABITypeAlignment(ST);
+
     SmallString<16> EltName = V->getName();
     EltName += ".elt";
     auto *Addr = SI.getPointerOperand();
     SmallString<16> AddrName = Addr->getName();
     AddrName += ".repack";
+
     auto *IdxType = Type::getInt32Ty(ST->getContext());
     auto *Zero = ConstantInt::get(IdxType, 0);
     for (unsigned i = 0; i < Count; i++) {
       Value *Indices[2] = {
         Zero,
         ConstantInt::get(IdxType, i),
       };
-      auto *Ptr = IC.Builder->CreateInBoundsGEP(ST, Addr, makeArrayRef(Indices), AddrName);
+      auto *Ptr = IC.Builder->CreateInBoundsGEP(ST, Addr,
+                                            makeArrayRef(Indices), AddrName);
       auto *Val = IC.Builder->CreateExtractValue(V, i, EltName);
-      IC.Builder->CreateStore(Val, Ptr);
+      auto EltAlign = MinAlign(Align, SL->getElementOffset(i));
+      IC.Builder->CreateAlignedStore(Val, Ptr, EltAlign);
     }
 
     return true;
   }
 
   if (auto *AT = dyn_cast<ArrayType>(T)) {
     // If the array only have one element, we unpack.
     if (AT->getNumElements() == 1) {

llvm/trunk/test/Transforms/InstCombine/unpack-fca.ll

 ; CHECK-NEXT: %tv.elt1 = getelementptr inbounds %struct.S, %struct.S* %s, i64 0, i32 1, i32 1
 ; CHECK-NEXT: %tv.unpack2 = load i32, i32* %tv.elt1, align 1
 ; CHECK-NEXT: ret i32 %tv.unpack2
   %t = getelementptr inbounds %struct.S, %struct.S* %s, i32 0, i32 1
   %tv = load %struct.T, %struct.T* %t, align 1
   %v = extractvalue %struct.T %tv, 1
   ret i32 %v
 }
+
+%struct.U = type {i8, i8, i8, i8, i8, i8, i8, i8, i64}
+
+define void @check_alignment(%struct.U* %u, %struct.U* %v) {
+; CHECK-LABEL: check_alignment
+; CHECK: load i8, i8* {{.*}}, align 8
+; CHECK: load i8, i8* {{.*}}, align 1
+; CHECK: load i8, i8* {{.*}}, align 2
+; CHECK: load i8, i8* {{.*}}, align 1
+; CHECK: load i8, i8* {{.*}}, align 4
+; CHECK: load i8, i8* {{.*}}, align 1
+; CHECK: load i8, i8* {{.*}}, align 2
+; CHECK: load i8, i8* {{.*}}, align 1
+; CHECK: load i64, i64* {{.*}}, align 8
+; CHECK: store i8 {{.*}}, i8* {{.*}}, align 8
+; CHECK: store i8 {{.*}}, i8* {{.*}}, align 1
+; CHECK: store i8 {{.*}}, i8* {{.*}}, align 2
+; CHECK: store i8 {{.*}}, i8* {{.*}}, align 1
+; CHECK: store i8 {{.*}}, i8* {{.*}}, align 4
+; CHECK: store i8 {{.*}}, i8* {{.*}}, align 1
+; CHECK: store i8 {{.*}}, i8* {{.*}}, align 2
+; CHECK: store i8 {{.*}}, i8* {{.*}}, align 1
+; CHECK: store i64 {{.*}}, i64* {{.*}}, align 8
+  %1 = load %struct.U, %struct.U* %u
+  store %struct.U %1, %struct.U* %v
+  ret void
+}