[PATCH] D33284: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGExpr.cpp:2256
             Address(CapLVal.getPointer(), getContext().getDeclAlign(VD)),
-            CapLVal.getType(), AlignmentSource::Decl);
+            CapLVal.getType(), LValueBaseInfo(AlignmentSource::Decl, MayAlias));
       }
----------------
kparzysz wrote:
> rjmccall wrote:
> > Hmm.  I think your side of this is right, but I'm not sure that the original code is correct to override with AlignmentSource::Decl, because the captured field might be a reference.  In fact, I'm not sure why this is overwriting the alignment, either; it should be able to just return the result of EmitCapturedFieldLValue, and that should be more correct for the same reason.  Do any tests break if you do that?
> There is one failure in check-clang: test/OpenMP/task_codegen.cpp.
> 
> 
> ```
> ; Function Attrs: noinline nounwind
> define internal i32 @.omp_task_entry..14(i32, %struct.kmp_task_t_with_privates.13* noalias) #1 {
> entry:
>   %.global_tid..addr.i = alloca i32, align 4
>   %.part_id..addr.i = alloca i32*, align 8
>   %.privates..addr.i = alloca i8*, align 8
>   %.copy_fn..addr.i = alloca void (i8*, ...)*, align 8
>   %.task_t..addr.i = alloca i8*, align 8
>   %__context.addr.i = alloca %struct.anon.12*, align 8
>   %.addr = alloca i32, align 4
>   %.addr1 = alloca %struct.kmp_task_t_with_privates.13*, align 8
>   store i32 %0, i32* %.addr, align 4
>   store %struct.kmp_task_t_with_privates.13* %1, %struct.kmp_task_t_with_privates.13** %.addr1, align 8
>   %2 = load i32, i32* %.addr, align 4
>   %3 = load %struct.kmp_task_t_with_privates.13*, %struct.kmp_task_t_with_privates.13** %.addr1, align 8
>   %4 = getelementptr inbounds %struct.kmp_task_t_with_privates.13, %struct.kmp_task_t_with_privates.13* %3, i32 0, i32 0
>   %5 = getelementptr inbounds %struct.kmp_task_t, %struct.kmp_task_t* %4, i32 0, i32 2
>   %6 = getelementptr inbounds %struct.kmp_task_t, %struct.kmp_task_t* %4, i32 0, i32 0
>   %7 = load i8*, i8** %6, align 8
>   %8 = bitcast i8* %7 to %struct.anon.12*
>   %9 = bitcast %struct.kmp_task_t_with_privates.13* %3 to i8*
>   store i32 %2, i32* %.global_tid..addr.i, align 4
>   store i32* %5, i32** %.part_id..addr.i, align 8
>   store i8* null, i8** %.privates..addr.i, align 8
>   store void (i8*, ...)* null, void (i8*, ...)** %.copy_fn..addr.i, align 8
>   store i8* %9, i8** %.task_t..addr.i, align 8
>   store %struct.anon.12* %8, %struct.anon.12** %__context.addr.i, align 8
>   %10 = load %struct.anon.12*, %struct.anon.12** %__context.addr.i, align 8
>   store i32 4, i32* @a, align 4
>   %11 = getelementptr inbounds %struct.anon.12, %struct.anon.12* %10, i32 0, i32 0
>   %ref.i = load i32*, i32** %11, align 8
>   store i32 5, i32* %ref.i, align 128      // XXX This alignment is 4 with that change.
>   ret i32 0
> }
> ```
> 
Ah, okay, I see what's happening here.  The field is actually *always* a reference, and and because it's just a reference the type doesn't have the declared alignment of the variable.

I think your change is probably the most reasonable approach.


Repository:
  rL LLVM

https://reviews.llvm.org/D33284