CodeGen: specify alignment for automatic variable initialization

[jfb]

Summary

Automatic variable initialization was generating default-aligned stores (which are deprecated) instead of using the known alignment from the alloca.

[efriedma]

lib/CodeGen/CGDecl.cpp

902 ↗

(On Diff #155090)

llvm::Value::getPointerAlignment is not the right way to compute the alignment (it tries to guess the alignment based on the IR). Please convert this to use a clang::CodeGen::Address instead.

[jfb]

• Use Address as suggested in review.

[jfb]

I updated the patch to use Address, and also use inbounds. This was a bit tricky because of the GEPs. I also updated tests which run into this codegen. Only the following tests actually hit this code, and not all check these stores:

Clang :: CodeGen/2004-03-09-LargeArrayInitializers.c
Clang :: CodeGen/indirect-goto.c
Clang :: CodeGen/init.c
Clang :: CodeGenCXX/float128-declarations.cpp
Clang :: CodeGenObjC/messages-2.m
Clang :: CodeGenObjC/messages.m
Clang :: CodeGenOpenCL/address-space-constant-initializers.cl
Clang :: CodeGenOpenCL/partial_initializer.cl
Clang :: CodeGenOpenCL/private-array-initialization.cl

[jfb]

• Fix silly naming and lookup.

[efriedma]

lib/CodeGen/CGBuilder.h

260 ↗

(On Diff #155287)

Not sure about the new helper. We already have CreateStructGEP and CreateConstArrayGEP which do approximately what you want.

lib/CodeGen/CGDecl.cpp

901 ↗

(On Diff #155287)

Builder.CreateStore(Init, Loc, isVolatile);

[jfb]

• Simplify CreateStore.

[jfb]

lib/CodeGen/CGBuilder.h

260 ↗

(On Diff #155287)

It's close to doing what we want, but not quite? It seems like the original code would have used them otherwise.

To use them we'd have to:

• branch on struct / array
  • for struct calculate the offset there (which the new helper does)
  • for array get the element size

Seems simpler to use GEP2_32 and more fool-proof to (internal to the helper) use GEP's own idea of what the offset is, no?

[efriedma]

LGTM

lib/CodeGen/CGBuilder.h

260 ↗

(On Diff #155287)

Okay.

(Actually, I think this ends up conservatively underestimating the alignment in some cases involving nested structs, but it's unlikely to matter.)

cfe/trunk/lib/CodeGen/CGBuilder.h

public:
   }
   Address CreateConstByteGEP(Address Addr, CharUnits Offset,
                              const llvm::Twine &Name = "") {
     assert(Addr.getElementType() == TypeCache.Int8Ty);
     return Address(CreateGEP(Addr.getPointer(), getSize(Offset), Name),
                    Addr.getAlignment().alignmentAtOffset(Offset));
   }
 
+  using CGBuilderBaseTy::CreateConstInBoundsGEP2_32;
+  Address CreateConstInBoundsGEP2_32(Address Addr, unsigned Idx0,
+                                      unsigned Idx1, const llvm::DataLayout &DL,
+                                      const llvm::Twine &Name = "") {
+    auto *GEP = cast<llvm::GetElementPtrInst>(CreateConstInBoundsGEP2_32(
+        Addr.getElementType(), Addr.getPointer(), Idx0, Idx1, Name));
+    llvm::APInt Offset(
+        DL.getIndexSizeInBits(Addr.getType()->getPointerAddressSpace()), 0,
+        /*IsSigned=*/true);
+    if (!GEP->accumulateConstantOffset(DL, Offset))
+      llvm_unreachable("offset of GEP with constants is always computable");
+    return Address(GEP, Addr.getAlignment().alignmentAtOffset(
+                            CharUnits::fromQuantity(Offset.getSExtValue())));
+  }
+
   llvm::Value *CreateConstInBoundsByteGEP(llvm::Value *Ptr, CharUnits Offset,
                                           const llvm::Twine &Name = "") {
     assert(Ptr->getType()->getPointerElementType() == TypeCache.Int8Ty);
     return CreateInBoundsGEP(Ptr, getSize(Offset), Name);
   }
   llvm::Value *CreateConstByteGEP(llvm::Value *Ptr, CharUnits Offset,
                                   const llvm::Twine &Name = "") {
     assert(Ptr->getType()->getPointerElementType() == TypeCache.Int8Ty);

cfe/trunk/lib/CodeGen/CGDecl.cpp

static bool canEmitInitWithFewStoresAfterMemset(llvm::Constant *Init,
 
   // Anything else is hard and scary.
   return false;
 }
 
 /// emitStoresForInitAfterMemset - For inits that
 /// canEmitInitWithFewStoresAfterMemset returned true for, emit the scalar
 /// stores that would be required.
-static void emitStoresForInitAfterMemset(llvm::Constant *Init, llvm::Value *Loc,
-                                         bool isVolatile, CGBuilderTy &Builder) {
+static void emitStoresForInitAfterMemset(CodeGenModule &CGM,
+                                         llvm::Constant *Init, Address Loc,
+                                         bool isVolatile,
+                                         CGBuilderTy &Builder) {
   assert(!Init->isNullValue() && !isa<llvm::UndefValue>(Init) &&
          "called emitStoresForInitAfterMemset for zero or undef value.");
 
   if (isa<llvm::ConstantInt>(Init) || isa<llvm::ConstantFP>(Init) ||
       isa<llvm::ConstantVector>(Init) || isa<llvm::BlockAddress>(Init) ||
       isa<llvm::ConstantExpr>(Init)) {
-    Builder.CreateDefaultAlignedStore(Init, Loc, isVolatile);
+    Builder.CreateStore(Init, Loc, isVolatile);
     return;
   }
 
   if (llvm::ConstantDataSequential *CDS =
           dyn_cast<llvm::ConstantDataSequential>(Init)) {
     for (unsigned i = 0, e = CDS->getNumElements(); i != e; ++i) {
       llvm::Constant *Elt = CDS->getElementAsConstant(i);
 
       // If necessary, get a pointer to the element and emit it.
       if (!Elt->isNullValue() && !isa<llvm::UndefValue>(Elt))
         emitStoresForInitAfterMemset(
-            Elt, Builder.CreateConstGEP2_32(Init->getType(), Loc, 0, i),
+            CGM, Elt,
+            Builder.CreateConstInBoundsGEP2_32(Loc, 0, i, CGM.getDataLayout()),
             isVolatile, Builder);
     }
     return;
   }
 
   assert((isa<llvm::ConstantStruct>(Init) || isa<llvm::ConstantArray>(Init)) &&
          "Unknown value type!");
 
   for (unsigned i = 0, e = Init->getNumOperands(); i != e; ++i) {
     llvm::Constant *Elt = cast<llvm::Constant>(Init->getOperand(i));
 
     // If necessary, get a pointer to the element and emit it.
     if (!Elt->isNullValue() && !isa<llvm::UndefValue>(Elt))
       emitStoresForInitAfterMemset(
-          Elt, Builder.CreateConstGEP2_32(Init->getType(), Loc, 0, i),
+          CGM, Elt,
+          Builder.CreateConstInBoundsGEP2_32(Loc, 0, i, CGM.getDataLayout()),
           isVolatile, Builder);
   }
 }
 
 /// shouldUseMemSetPlusStoresToInitialize - Decide whether we should use memset
 /// plus some stores to initialize a local variable instead of using a memcpy
 /// from a constant global.  It is beneficial to use memset if the global is all
 /// zeros, or mostly zeros and large.
void CodeGenFunction::EmitAutoVarInit(const AutoVarEmission &emission) {
   if (shouldUseMemSetPlusStoresToInitialize(constant,
                 CGM.getDataLayout().getTypeAllocSize(constant->getType()))) {
     Builder.CreateMemSet(Loc, llvm::ConstantInt::get(Int8Ty, 0), SizeVal,
                          isVolatile);
     // Zero and undef don't require a stores.
     if (!constant->isNullValue() && !isa<llvm::UndefValue>(constant)) {
       Loc = Builder.CreateBitCast(Loc,
         constant->getType()->getPointerTo(Loc.getAddressSpace()));
-      emitStoresForInitAfterMemset(constant, Loc.getPointer(),
-                                   isVolatile, Builder);
+      emitStoresForInitAfterMemset(CGM, constant, Loc, isVolatile, Builder);
     }
   } else {
     // Otherwise, create a temporary global with the initializer then
     // memcpy from the global to the alloca.
     std::string Name = getStaticDeclName(CGM, D);
     unsigned AS = CGM.getContext().getTargetAddressSpace(
         CGM.getStringLiteralAddressSpace());
     BP = llvm::PointerType::getInt8PtrTy(getLLVMContext(), AS);

cfe/trunk/test/CodeGen/init.c

struct Huge {
   int arr[1000 * 1000 * 1000];
 } huge_struct = {1, {2, 0, 0, 0}};
 
 
 // PR279 comment #3
 char test8(int X) {
   char str[100000] = "abc"; // tail should be memset.
   return str[X];
-// CHECK: @test8(
+  // CHECK-LABEL: @test8(
   // CHECK: call void @llvm.memset
-// CHECK: store i8 97
-// CHECK: store i8 98
-// CHECK: store i8 99
+  // CHECK: store i8 97, i8* %{{[0-9]*}}, align 1
+  // CHECK: store i8 98, i8* %{{[0-9]*}}, align 1
+  // CHECK: store i8 99, i8* %{{[0-9]*}}, align 1
   // CHECK-NOT: getelementptr
   // CHECK: load
 }
 
 void bar(void*);
 
 // PR279
-int test9(int X) {
+void test9(int X) {
   int Arr[100] = { X };     // Should use memset
   bar(Arr);
-// CHECK: @test9
+  // CHECK-LABEL: @test9(
   // CHECK: call void @llvm.memset
   // CHECK-NOT: store i32 0
   // CHECK: call void @bar
 }
 
 struct a {
   int a, b, c, d, e, f, g, h, i, j, k, *p;
 };
 
 struct b {
   struct a a,b,c,d,e,f,g;
 };
 
-int test10(int X) {
+void test10(int X) {
   struct b S = { .a.a = X, .d.e = X, .f.e = 0, .f.f = 0, .f.p = 0 };
   bar(&S);
 
-  // CHECK: @test10
+  // CHECK-LABEL: @test10(
   // CHECK: call void @llvm.memset
   // CHECK-NOT: store i32 0
   // CHECK: call void @bar
 }
 
 
 // PR9257
 struct test11S {
   int A[10];
 };
 void test11(struct test11S *P) {
   *P = (struct test11S) { .A = { [0 ... 3] = 4 } };
-  // CHECK: @test11
-  // CHECK: store i32 4
-  // CHECK: store i32 4
-  // CHECK: store i32 4
-  // CHECK: store i32 4
+  // CHECK-LABEL: @test11(
+  // CHECK: store i32 4, i32* %{{.*}}, align 4
+  // CHECK: store i32 4, i32* %{{.*}}, align 4
+  // CHECK: store i32 4, i32* %{{.*}}, align 4
+  // CHECK: store i32 4, i32* %{{.*}}, align 4
   // CHECK: ret void
 }
 
 
 // Verify that we can convert a recursive struct with a memory that returns
 // an instance of the struct we're converting.
 struct test12 {
   struct test12 (*p)(void);
 } test12g;
 
 
 void test13(int x) {
   struct X { int a; int b : 10; int c; };
   struct X y = {.c = x};
-  // CHECK: @test13
+  // CHECK-LABEL: @test13(
   // CHECK: and i16 {{.*}}, -1024
 }
 
-// CHECK-LABEL: @PR20473
+// CHECK-LABEL: @PR20473(
 void PR20473() {
   // CHECK: memcpy{{.*}}getelementptr inbounds ([2 x i8], [2 x i8]* @
   bar((char[2]) {""});
   // CHECK: memcpy{{.*}}getelementptr inbounds ([3 x i8], [3 x i8]* @
   bar((char[3]) {""});
 }
 
 // Test that we initialize large member arrays by copying from a global and not
 // with a series of stores.
 struct S14 { int a[16]; };
 
 void test14(struct S14 *s14) {
-// CHECK-LABEL: @test14
+  // CHECK-LABEL: @test14(
   // CHECK: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 {{.*}}, i8* align 4 {{.*}} [[INIT14]] {{.*}}, i32 64, i1 false)
   // CHECK-NOT: store
   // CHECK: ret void
   *s14 = (struct S14) { { [5 ... 11] = 17 } };
 }

cfe/trunk/test/CodeGenOpenCL/partial_initializer.cl

void f(void) {
   // CHECK: %[[V1:.*]] = alloca <4 x i32>, align 16
   // CHECK: %[[compoundliteral:.*]] = alloca <4 x i32>, align 16
   // CHECK: %[[compoundliteral1:.*]] = alloca <2 x i32>, align 8
   // CHECK: %[[V2:.*]] = alloca <4 x i32>, align 16
 
   // CHECK: %[[v0:.*]] = bitcast [6 x [6 x float]]* %A to i8*
   // CHECK: call void @llvm.memset.p0i8.i32(i8* align 4 %[[v0]], i8 0, i32 144, i1 false)
   // CHECK: %[[v1:.*]] = bitcast i8* %[[v0]] to [6 x [6 x float]]*
-  // CHECK: %[[v2:.*]] = getelementptr [6 x [6 x float]], [6 x [6 x float]]* %[[v1]], i32 0, i32 0
-  // CHECK: %[[v3:.*]] = getelementptr [6 x float], [6 x float]* %[[v2]], i32 0, i32 0
-  // CHECK: store float 1.000000e+00, float* %[[v3]]
-  // CHECK: %[[v4:.*]] = getelementptr [6 x float], [6 x float]* %[[v2]], i32 0, i32 1
-  // CHECK: store float 2.000000e+00, float* %[[v4]]
+  // CHECK: %[[v2:.*]] = getelementptr inbounds [6 x [6 x float]], [6 x [6 x float]]* %[[v1]], i32 0, i32 0
+  // CHECK: %[[v3:.*]] = getelementptr inbounds [6 x float], [6 x float]* %[[v2]], i32 0, i32 0
+  // CHECK: store float 1.000000e+00, float* %[[v3]], align 4
+  // CHECK: %[[v4:.*]] = getelementptr inbounds [6 x float], [6 x float]* %[[v2]], i32 0, i32 1
+  // CHECK: store float 2.000000e+00, float* %[[v4]], align 4
   float A[6][6]  = {1.0f, 2.0f};
 
   // CHECK: %[[v5:.*]] = bitcast %struct.StrucTy* %S to i8*
   // CHECK: call void @llvm.memcpy.p0i8.p2i8.i32(i8* align 4 %[[v5]], i8 addrspace(2)* align 4 bitcast (%struct.StrucTy addrspace(2)* @f.S to i8 addrspace(2)*), i32 12, i1 false)
   StrucTy S = {1, 2};
 
   // CHECK: store <2 x i32> <i32 1, i32 2>, <2 x i32>* %[[compoundliteral1]], align 8
   // CHECK: %[[v6:.*]] = load <2 x i32>, <2 x i32>* %[[compoundliteral1]], align 8