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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits