[PATCH] D33284: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource
John McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed May 17 11:03:20 PDT 2017
rjmccall added a comment.
Generally looks great, thanks!
================
Comment at: lib/CodeGen/CGBlocks.cpp:923
+ MakeAddrLValue(blockField, type,
+ LValueBaseInfo(AlignmentSource::Decl, false)),
/*captured by init*/ false);
----------------
Please indent this line a little more to make it clear that it's part of the same argument.
================
Comment at: lib/CodeGen/CGExpr.cpp:884
+ if (BaseInfo)
+ BaseInfo->setAlignmentSource(Source);
}
----------------
Places that fall back to the type of a pointer/reference/whatever should probably compute the entire BaseInfo from the type. For example, if we wanted to properly support attribute may_alias on types, we'd need to compute it here. So I think you should probably change getNaturalTypeAlignment and getNaturalPointeeTypeAlignment to produce an entire BaseInfo, even if you always fill it in with may_alias=false for now.
Here you'll also need to merge the BaseInfos of the original pointer and the new one. You should probably have a method on LValueBaseInfo that does that, called maybe mergeForCast?
================
Comment at: lib/CodeGen/CGExpr.cpp:2256
Address(CapLVal.getPointer(), getContext().getDeclAlign(VD)),
- CapLVal.getType(), AlignmentSource::Decl);
+ CapLVal.getType(), LValueBaseInfo(AlignmentSource::Decl, MayAlias));
}
----------------
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?
Repository:
rL LLVM
https://reviews.llvm.org/D33284
More information about the cfe-commits
mailing list