Support the assume_aligned function attribute

[hfinkel]

Summary

In addition to __builtin_assume_aligned, GCC also supports an assume_aligned attribute which specifies the alignment (and optional offset) of the function's return value. Building on the llvm.assume patches, and also functionality added in http://reviews.llvm.org/D149, this patch implements support for the assume_aligned attribute in Clang.

[rsmith]

Somewhat unrelated to this patch in particular, but we should add a UBSan check to catch failed assumptions.

include/clang/Basic/Attr.td

867

These should probably both be ExprArguments, in order to support dependent alignment/offset expressions in templates.

868

Please add documentation.

lib/CodeGen/CGExpr.cpp

3352

Please add parens around this if.

lib/Sema/SemaDeclAttr.cpp

1219–1227

I think there's a helper function for this sequence of checks/diagnostics. If not, there should be.

1232–1235

Do you really need to check that the Offset fits into an uint32_t? Truncating the offset to the width of Alignment seems correct in all cases.

[rsmith]

lib/CodeGen/CGExpr.cpp

3352

s/parens/braces/ =)

[hfinkel]

Updated per review comments from Richard and Aaron (and rebased).

[rsmith]

lib/CodeGen/CGExpr.cpp

3350–3364

Is this really the right place for this code, rather than in the called EmitCall function? Various places call the lower-level overload directly.

lib/Sema/SemaDeclAttr.cpp

1104–1105

Update this comment, please.

1104–1105

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".)

1234

You don't need the type-dependency check here.

1256

Likewise here.

lib/Sema/SemaTemplateInstantiateDecl.cpp

203–205 ↗

(On Diff #13228)

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.

[hfinkel]

lib/CodeGen/CGExpr.cpp

3350–3364

When I first looked, I thought that there were no other relevant callers, but maybe EmitCallAndReturnForThunk and EmitForwardingCallToLambda would need this handling too?

Because of the way that the lower-level EmitCall is structured, I'd need to wrap it (it has too many early returns). What would I call it?

lib/Sema/SemaDeclAttr.cpp

1104–1105

Nice :-) -- Honestly, I don't have a strong opinion regarding whether or not we strip references, but I like the idea of consistency between the two attributes. The set of returned pointers about which I can assert something about the alignment should be the same as that about which I can assert a nonnull property.

That having been said, it looks like the CodeGen for returns_nonnull that adds the nonnull IR attribute (and the associated UBSan code) does not handle references-to-pointers correctly. I'll fix that as a follow-up too.

lib/Sema/SemaTemplateInstantiateDecl.cpp

203–205 ↗

(On Diff #13228)

Interestingly, the current code does generate an error:

invalid application of 'sizeof' to a function type

Removing the type-dependent checks does not seem to change any of the current behavior.

[rsmith]

lib/CodeGen/CGExpr.cpp

3350–3364

EmitCXXMemberOrOperatorCall, EmitCXXMemberPointerCallExpr, and EmitNewDeleteCall should also get this handling.

Maybe factor out the switch that produces the returned RValue from the call?

lib/Sema/SemaDeclAttr.cpp

1104–1105

It seems really weird for either of these to look through references to the referenced pointer. And it's ambiguous: does the attribute apply to the reference or to the pointer that the reference refers to? (For the nonnull attribute, GCC and Clang make different choices here, at least in the cases where you can persuade GCC not to reject the attribute!)

lib/Sema/SemaTemplateInstantiateDecl.cpp

203–205 ↗

(On Diff #13228)

Ha, sorry, bitten by a parse ambiguity. Six closing parens wasn't enough; let's go up to seven:

template<typename T> __attribute__((assume_aligned(sizeof((int(T())))))) T *f();
void *p = f<void>();

[hfinkel]

lib/Sema/SemaDeclAttr.cpp

1104–1105

Indeed, unlike nonnull, this does become ambiguous. Perhaps it is best to prohibit it?

How do Clang and GCC make different choices for nonnull; is that a bug? Because a reference cannot be null, it would seem that it should apply to the pointer being bound.

[rsmith]

lib/Sema/SemaDeclAttr.cpp

1104–1105

Here's what I can piece together:

• GCC does not intend to support nonnull on reference parameters. It rejects nonnull(N) where parameter N is a reference, *but* if you apply nonnull with no N, it applies to both pointer and reference parameters, and happens to have the semantics of checking for "null references".
• Clang made up its own thing in response to some non-publicly-visible radar issue, where it treats the nonnull attribute on references-to-pointers as applying to the pointer within the reference. This makes little sense to me, but there it is. We naturally can't optimize on the basis of nonnull on a reference-to-pointer like we can on a real pointer parameter/return value (we have no IR representation for that).

It seems our options are to either not support assume_aligned on references (which would be a shame, because it's natural and useful there just as it is for pointers), or make nonnull and assume_aligned inconsistent on pointers-to-references, or remove our (dubious, IMO) extension of the GNU nonnull semantics. I'm not happy with the third option since I don't know what the motivation for the extension is nor how many people might be relying on it, but either of the first two seem fine to me. I don't think the connection between nonnull and assume_aligned is strong enough that the difference would be jarring for people.

[hfinkel]

lib/CodeGen/CGExpr.cpp

3350–3364

I tried using a lambda, let me know what you think.

lib/Sema/SemaDeclAttr.cpp

1104–1105

Alright, I'll go with option 1, we'll have it apply to references in the natural way.

lib/Sema/SemaTemplateInstantiateDecl.cpp

203–205 ↗

(On Diff #13228)

Test case added.

[hfinkel]

Updated per Richard's review.

[hfinkel]

Ping.

[hfinkel]

r218500, thanks!

include/clang/Basic/Attr.td

 
 def ReturnsNonNull : InheritableAttr {
   let Spellings = [GCC<"returns_nonnull">];
   let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag,
                              "ExpectedFunctionOrMethod">;
   let Documentation = [Undocumented];
 }
 
+def AssumeAligned : InheritableAttr {
+  let Spellings = [GCC<"assume_aligned">];
+  let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag,
+                             "ExpectedFunctionOrMethod">;
+  let Args = [IntArgument<"Alignment">, DefaultIntArgument<"Offset", 0>];
+  let Documentation = [Undocumented];
+}
+
 def NoReturn : InheritableAttr {
   let Spellings = [GCC<"noreturn">, Declspec<"noreturn">];
   // FIXME: Does GCC allow this on the function instead?
   let Documentation = [Undocumented];
 }
 
 def NoInstrumentFunction : InheritableAttr {
   let Spellings = [GCC<"no_instrument_function">];

lib/CodeGen/CGExpr.cpp

RValue CodeGenFunction::EmitCall(QualType CalleeType, llvm::Value *Callee,
   // call.  The way we make this work is to cast to the exact type
   // of the promoted arguments.
   if (isa<FunctionNoProtoType>(FnType)) {
     llvm::Type *CalleeTy = getTypes().GetFunctionType(FnInfo);
     CalleeTy = CalleeTy->getPointerTo();
     Callee = Builder.CreateBitCast(Callee, CalleeTy, "callee.knr.cast");
   }
 
-  return EmitCall(FnInfo, Callee, ReturnValue, Args, TargetDecl);
+  RValue Ret = EmitCall(FnInfo, Callee, ReturnValue, Args, TargetDecl);
+
+  if (Ret.isScalar() && TargetDecl)
+    if (const AssumeAlignedAttr *AA =
+          TargetDecl->getAttr<AssumeAlignedAttr>()) {
+      llvm::Value *OffsetValue = nullptr;
+      if (int Offset = AA->getOffset())
+        OffsetValue = llvm::ConstantInt::get(IntPtrTy, Offset);
+
+      EmitAlignmentAssumption(Ret.getScalarVal(), AA->getAlignment(),
+                              OffsetValue);
+    }
+
+  return Ret;
 }
 
 LValue CodeGenFunction::
 EmitPointerToDataMemberBinaryExpr(const BinaryOperator *E) {
   llvm::Value *BaseV;
   if (E->getOpcode() == BO_PtrMemI)
     BaseV = EmitScalarExpr(E->getLHS());
   else
     BaseV = EmitLValue(E->getLHS()).getAddress();

lib/Sema/SemaDeclAttr.cpp

 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Scope.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/MathExtras.h"
 using namespace clang;
 using namespace sema;
 
 namespace AttributeLangSupport {
   enum LANG {
     C,
     Cpp,
     ObjC
S.Diag(Attr.getLoc(),
                                : diag::err_iboutletcollection_type) << QT;
     return;
   }
 
   D->addAttr(::new (S.Context)
              IBOutletCollectionAttr(Attr.getRange(), S.Context, QTLoc,
                                     Attr.getAttributeSpellingListIndex()));
 }
 
 static void possibleTransparentUnionPointerType(QualType &T) {
   if (const RecordType *UT = T->getAsUnionType())
     if (UT && UT->getDecl()->hasAttr<TransparentUnionAttr>()) {
       RecordDecl *UD = UT->getDecl();
       for (const auto *I : UD->fields()) {
         QualType QT = I->getType();
         if (QT->isAnyPointerType() || QT->isBlockPointerType()) {
           T = QT;
           return;
         }
       }
     }
 }
 
-static bool attrNonNullArgCheck(Sema &S, QualType T, const AttributeList &Attr,
+static bool attrPointerArgCheck(Sema &S, QualType T, const AttributeList &Attr,
                                 SourceRange R, bool isReturnValue = false) {
   T = T.getNonReferenceType();
   possibleTransparentUnionPointerType(T);
 
   if (!T->isAnyPointerType() && !T->isBlockPointerType()) {
     S.Diag(Attr.getLoc(),
            isReturnValue ? diag::warn_attribute_return_pointers_only
                          : diag::warn_attribute_pointers_only)
       << Attr.getName() << R;
     return false;
   }
   return true;
 }
 
 static void handleNonNullAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   SmallVector<unsigned, 8> NonNullArgs;
   for (unsigned i = 0; i < Attr.getNumArgs(); ++i) {
     Expr *Ex = Attr.getArgAsExpr(i);
     uint64_t Idx;
     if (!checkFunctionOrMethodParameterIndex(S, D, Attr, i + 1, Ex, Idx))
       return;
 
     // Is the function argument a pointer type?
     // FIXME: Should also highlight argument in decl in the diagnostic.
-    if (!attrNonNullArgCheck(S, getFunctionOrMethodParamType(D, Idx), Attr,
+    if (!attrPointerArgCheck(S, getFunctionOrMethodParamType(D, Idx), Attr,
                              Ex->getSourceRange()))
       continue;
 
     NonNullArgs.push_back(Idx);
   }
 
   // If no arguments were specified to __attribute__((nonnull)) then all pointer
   // arguments have a nonnull attribute.
if (Attr.getNumArgs() > 0) {
     } else {
       S.Diag(Attr.getLoc(), diag::warn_attribute_nonnull_parm_no_args)
         << D->getSourceRange();
     }
     return;
   }
 
   // Is the argument a pointer type?
-  if (!attrNonNullArgCheck(S, D->getType(), Attr, D->getSourceRange()))
+  if (!attrPointerArgCheck(S, D->getType(), Attr, D->getSourceRange()))
     return;
 
   D->addAttr(::new (S.Context)
              NonNullAttr(Attr.getRange(), S.Context, nullptr, 0,
                          Attr.getAttributeSpellingListIndex()));
 }
 
 static void handleReturnsNonNullAttr(Sema &S, Decl *D,
                                      const AttributeList &Attr) {
   QualType ResultType = getFunctionOrMethodResultType(D);
-  if (!attrNonNullArgCheck(S, ResultType, Attr, Attr.getRange(),
+  if (!attrPointerArgCheck(S, ResultType, Attr, Attr.getRange(),
                            /* isReturnValue */ true))
     return;
 
   D->addAttr(::new (S.Context)
             ReturnsNonNullAttr(Attr.getRange(), S.Context,
                                Attr.getAttributeSpellingListIndex()));
 }
 
+static void handleAssumeAlignedAttr(Sema &S, Decl *D,
+                                    const AttributeList &Attr) {
+  QualType ResultType = getFunctionOrMethodResultType(D);
+  if (!attrPointerArgCheck(S, ResultType, Attr, Attr.getRange(),
+                           /* isReturnValue */ true))
+    return;
+
+  if (Attr.getNumArgs() > 2) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_too_many_arguments)
+      << Attr.getName() << 2;
+    return;
+  } else if (Attr.getNumArgs() < 1) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_too_few_arguments)
+      << Attr.getName() << 1;
+    return;
+  }
+
+  uint32_t Alignment, Offset = 0;
+  if (!checkUInt32Argument(S, Attr, Attr.getArgAsExpr(0), Alignment, 1))
+    return;
+  if (Attr.getNumArgs() > 1 && !checkUInt32Argument(S, Attr,
+                                                    Attr.getArgAsExpr(1),
+                                                    Offset, 2))
+    return;
+
+  if (!llvm::isPowerOf2_32(Alignment)) {
+    S.Diag(Attr.getLoc(), diag::err_alignment_not_power_of_two)
+      << Attr.getArgAsExpr(0)->getSourceRange();
+    return;
+  }
+
+  D->addAttr(::new (S.Context)
+            AssumeAlignedAttr(Attr.getRange(), S.Context, Alignment, Offset,
+                              Attr.getAttributeSpellingListIndex()));
+}
+
 static void handleOwnershipAttr(Sema &S, Decl *D, const AttributeList &AL) {
   // This attribute must be applied to a function declaration. The first
   // argument to the attribute must be an identifier, the name of the resource,
   // for example: malloc. The following arguments must be argument indexes, the
   // arguments must be of integer type for Returns, otherwise of pointer type.
   // The difference between Holds and Takes is that a pointer may still be used
   // after being held. free() should be __attribute((ownership_takes)), whereas
   // a list append function may well be __attribute((ownership_holds)).
 
   if (!AL.isArgIdent(0)) {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type)
       << AL.getName() << 1 << AANT_ArgumentIdentifier;
     return;
   }
 
   // Figure out our Kind.
   OwnershipAttr::OwnershipKind K =
case AttributeList::AT_NonNull:
     if (ParmVarDecl *PVD = dyn_cast<ParmVarDecl>(D))
       handleNonNullAttrParameter(S, PVD, Attr);
     else
       handleNonNullAttr(S, D, Attr);
     break;
   case AttributeList::AT_ReturnsNonNull:
     handleReturnsNonNullAttr(S, D, Attr);
     break;
+  case AttributeList::AT_AssumeAligned:
+    handleAssumeAlignedAttr(S, D, Attr);
+    break;
   case AttributeList::AT_Overloadable:
     handleSimpleAttribute<OverloadableAttr>(S, D, Attr);
     break;
   case AttributeList::AT_Ownership:
     handleOwnershipAttr(S, D, Attr);
     break;
   case AttributeList::AT_Cold:
     handleColdAttr(S, D, Attr);

test/CodeGen/builtin-assume-aligned.c

 // CHECK: %offsetptr = sub i64 %ptrint, %conv
 // CHECK: %maskedptr = and i64 %offsetptr, 31
 // CHECK: %maskcond = icmp eq i64 %maskedptr, 0
 // CHECK: call void @llvm.assume(i1 %maskcond)
   a = __builtin_assume_aligned(a, 32, b);
   return a[0];
 }
 
+int *m1() __attribute__((assume_aligned(64)));
+
+// CHECK-LABEL: @test5
+int test5() {
+  return *m1();
+// CHECK: %ptrint = ptrtoint
+// CHECK: %maskedptr = and i64 %ptrint, 63
+// CHECK: %maskcond = icmp eq i64 %maskedptr, 0
+// CHECK: call void @llvm.assume(i1 %maskcond)
+}
+
+int *m2() __attribute__((assume_aligned(64, 12)));
+
+// CHECK-LABEL: @test6
+int test6() {
+  return *m2();
+// CHECK: %ptrint = ptrtoint
+// CHECK: %offsetptr = sub i64 %ptrint, 12
+// CHECK: %maskedptr = and i64 %offsetptr, 63
+// CHECK: %maskcond = icmp eq i64 %maskedptr, 0
+// CHECK: call void @llvm.assume(i1 %maskcond)
+}
+

test/Sema/builtin-assume-aligned.c

int test7(int *a) {
   return a[0];
 }
 
 int test8(int *a, int j) {
   a = __builtin_assume_aligned(a, j); // expected-error {{must be a constant integer}}
   return a[0];
 }
 
+void test_void_assume_aligned(void) __attribute__((assume_aligned(32))); // expected-warning {{'assume_aligned' attribute only applies to return values that are pointers}}
+int test_int_assume_aligned(void) __attribute__((assume_aligned(16))); // expected-warning {{'assume_aligned' attribute only applies to return values that are pointers}}
+void *test_ptr_assume_aligned(void) __attribute__((assume_aligned(64))); // no-warning
+
+int j __attribute__((assume_aligned(8))); // expected-warning {{'assume_aligned' attribute only applies to functions and methods}}
+void *test_no_fn_proto() __attribute__((assume_aligned(32))); // no-warning
+void *test_with_fn_proto(void) __attribute__((assume_aligned(128))); // no-warning
+
+void *test_no_fn_proto() __attribute__((assume_aligned(31))); // expected-error {{requested alignment is not a power of 2}}
+void *test_no_fn_proto() __attribute__((assume_aligned(32, 73))); // no-warning
+
+void *test_no_fn_proto() __attribute__((assume_aligned)); // expected-error {{'assume_aligned' attribute takes at least 1 argument}}
+void *test_no_fn_proto() __attribute__((assume_aligned())); // expected-error {{'assume_aligned' attribute takes at least 1 argument}}
+void *test_no_fn_proto() __attribute__((assume_aligned(32, 45, 37))); // expected-error {{'assume_aligned' attribute takes no more than 2 arguments}}
+