[PATCH] Support the assume_aligned function attribute
Richard Smith
richard at metafoo.co.uk
Wed Sep 3 17:09:41 PDT 2014
================
Comment at: lib/CodeGen/CGExpr.cpp:3379-3382
@@ -3378,2 +3378,6 @@
- return EmitCall(FnInfo, Callee, ReturnValue, Args, TargetDecl);
+ RValue Ret = EmitCall(FnInfo, Callee, ReturnValue, Args, TargetDecl);
+
+ if (Ret.isScalar() && TargetDecl) {
+ if (const auto *AA = TargetDecl->getAttr<AssumeAlignedAttr>()) {
+ llvm::Value *OffsetValue = nullptr;
----------------
Is this really the right place for this code, rather than in the called `EmitCall` function? Various places call the lower-level overload directly.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1130
@@ -1129,2 +1129,3 @@
+bool Sema::isValidPointerAttrType(QualType T) {
T = T.getNonReferenceType();
----------------
Is this really appropriate for your attribute? (It makes almost no sense for `__attribute__((nonnull))` either, but I couldn't find anyone at Apple to tell me what the relevant radar says, so I have no idea why we have this "feature".)
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1132-1133
@@ -1130,4 +1131,4 @@
// The nonnull attribute can be applied to a transparent union that
// contains a pointer type.
if (const RecordType *UT = T->getAsUnionType()) {
----------------
Update this comment, please.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1263
@@ +1262,3 @@
+
+ if (!E->isTypeDependent() && !E->isValueDependent()) {
+ llvm::APSInt I(64);
----------------
You don't need the type-dependency check here.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1285
@@ +1284,3 @@
+ if (OE) {
+ if (!OE->isTypeDependent() && !OE->isValueDependent()) {
+ llvm::APSInt I(64);
----------------
Likewise here.
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:203-205
@@ +202,5 @@
+ const AssumeAlignedAttr *AssumeAligned = dyn_cast<AssumeAlignedAttr>(TmplAttr);
+ if (AssumeAligned && (AssumeAligned->getAlignment()->isValueDependent() ||
+ (AssumeAligned->getOffset() &&
+ AssumeAligned->getOffset()->isValueDependent()))) {
+ instantiateDependentAssumeAlignedAttr(*this, TemplateArgs, AssumeAligned, New);
----------------
You shouldn't have these 'is dependent' checks here; the other code above and below is wrong to include them. Testcase:
template<typename T> __attribute__((assume_aligned(sizeof(int(T()))))) T *f();
void *p = f<void>();
This should result in a hard error, because `int(void())` is invalid, but I think you'll accept it, because `sizeof(int(T()))` is not value-dependent, so you never actually perform the substitution.
http://reviews.llvm.org/D4601
More information about the cfe-commits
mailing list