[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