Don't promote load alignment from packed structure elements

[pete]

Summary

Hi all

Looks like http://reviews.llvm.org/D8339 breaks loads from packed structs.

The problem is that when we create a new load of the aggregate type, we didn't set its alignment. So a load of a packed int gets promote from align 1 to align 4.

I was told about this offline by Neil Henning from Codeplay. He was able to describe the problem well enough that I could make this small test case.

He also mentioned that this is breaking his code in a way where he would really like to see this get in to 3.8. No idea if thats possible, but just putting that out there to see if that would be ok.

Thanks,
Pete

[majnemer]

LGTM

[pete]

Thanks for the very quick review David.

How do you feel about taking this in 3.8?

Cheers,
Pete

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

if (auto *ST = dyn_cast<StructType>(T)) {
     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), EltName);
-      auto *L = IC.Builder->CreateLoad(ST->getTypeAtIndex(i), Ptr, LoadName);
+      auto *L = IC.Builder->CreateAlignedLoad(Ptr, LI.getAlignment(),
+                                              LoadName);
       V = IC.Builder->CreateInsertValue(V, L, i);
     }
 
     V->setName(Name);
     return IC.replaceInstUsesWith(LI, V);
   }
 
   if (auto *AT = dyn_cast<ArrayType>(T)) {

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

 ; CHECK-NEXT: store i8* null, i8** [[GEP1]], align 8
 ; CHECK-NEXT: [[GEP2:%[a-z0-9\.]+]] = getelementptr inbounds %B, %B* %b.ptr, i64 0, i32 1
 ; CHECK-NEXT: store i64 42, i64* [[GEP2]], align 8
 ; CHECK-NEXT: ret %B { i8* null, i64 42 }
   store %B { i8* null, i64 42 }, %B* %b.ptr, align 8
   %1 = load %B, %B* %b.ptr, align 8
   ret %B %1
 }
+
+%struct.S = type <{ i8, %struct.T }>
+%struct.T = type { i32, i32 }
+
+; Make sure that we do not increase alignment of packed struct element
+define i32 @packed_alignment(%struct.S* dereferenceable(9) %s) {
+; CHECK-LABEL: packed_alignment
+; 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
+}