[PATCH] align_value attribute in Clang
Aaron Ballman
aaron.ballman at gmail.com
Tue Aug 26 07:58:50 PDT 2014
On Mon, Aug 25, 2014 at 4:51 PM, hfinkel at anl.gov <hfinkel at anl.gov> wrote:
> An updated patch, accounting for review comments -- but this does not yet properly treat the attribute as a type attribute (so that it will be inherited by auto, etc.). I still need to work on that aspect, and I'll post another revision once I have those semantics working.
>
> http://reviews.llvm.org/D4635
>
> Files:
> include/clang/Basic/Attr.td
> include/clang/Basic/AttrDocs.td
> include/clang/Basic/DiagnosticSemaKinds.td
> include/clang/Sema/AttributeList.h
> include/clang/Sema/Sema.h
> lib/CodeGen/CGCall.cpp
> lib/Sema/SemaDeclAttr.cpp
> lib/Sema/SemaTemplateInstantiateDecl.cpp
> test/CodeGen/align_value.cpp
> test/Sema/align_value.c
> test/SemaCXX/align_value.cpp
This patch looks like it's coming along nicely! Comments below.
> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td
> +++ include/clang/Basic/Attr.td
> @@ -354,6 +354,23 @@
> let Documentation = [Undocumented];
> }
>
> +def AlignValue : Attr {
> + let Spellings = [
> + // Unfortunately, this is semantically an assertion, not a
> + // directive (something else must ensure the alignment), so
> + // aligned_value is a probably a better name. We might want
> + // to add an aligned_value spelling in the future (and a
> + // corresponding C++ attribute), but this can be done later
> + // once we decide if we also want them to have
> + // slightly-different semantics than Intel's align_value.
> + GNU<"align_value">
> + // Intel's compiler on Windows also supports:
> + // , Declspec<"align_value">
> + ];
> + let Args = [ExprArgument<"Alignment">];
> + let Documentation = [AlignValueDocs];
Since it may apply only to typedefs and variables, would it make sense
to add a Subjects?
> +}
> +
> def AlignMac68k : InheritableAttr {
> // This attribute has no spellings as it is only ever created implicitly.
> let Spellings = [];
> Index: include/clang/Basic/AttrDocs.td
> ===================================================================
> --- include/clang/Basic/AttrDocs.td
> +++ include/clang/Basic/AttrDocs.td
> @@ -987,6 +987,25 @@
> }];
> }
>
> +def AlignValueDocs : Documentation {
> + let Category = DocCatType;
> + let Content = [{
> +The align_value attribute can be added to the typedef of a pointer type or the
> +declaration of a variable of pointer or reference type. It specifies that the
> +pointer will point to, or the reference will bind to, only objects with at
> +least the provided alignment. This alignment value must be some positive power
> +of 2.
> +
> + .. code-block:: c
> +
> + typedef double * aligned_double_ptr __attribute__((align_value(64)));
> + void foo(double & x __attribute__((align_value(128)),
> + aligned_double_ptr y) { ... }
> +
> +A C++ attribute clang::align_value is also provided.
This doesn't appear to be the case judging by the spelling for the
attribute. There's no CXX11 spelling specified.
> + }];
> +}
> +
> def MSInheritanceDocs : Documentation {
> let Category = DocCatType;
> let Heading = "__single_inhertiance, __multiple_inheritance, __virtual_inheritance";
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td
> +++ include/clang/Basic/DiagnosticSemaKinds.td
> @@ -1908,8 +1908,12 @@
> "Neon vector size must be 64 or 128 bits">;
> def err_attribute_unsupported : Error<
> "%0 attribute is not supported for this target">;
> +// The err_*_attribute_argument_not_int are seperate because they're used by
> +// VerifyIntegerConstantExpression.
> def err_aligned_attribute_argument_not_int : Error<
> "'aligned' attribute requires integer constant">;
> +def err_align_value_attribute_argument_not_int : Error<
> + "'align_value' attribute requires integer constant">;
> def err_alignas_attribute_wrong_decl_type : Error<
> "%0 attribute cannot be applied to a %select{function parameter|"
> "variable with 'register' storage class|'catch' variable|bit-field}1">;
> @@ -1945,6 +1949,9 @@
> def warn_attribute_return_pointers_only : Warning<
> "%0 attribute only applies to return values that are pointers">,
> InGroup<IgnoredAttributes>;
> +def warn_attribute_pointer_or_reference_only : Warning<
> + "%0 attribute only applies to a pointer or reference (%1 is invalid)">,
> + InGroup<IgnoredAttributes>;
> def err_attribute_no_member_pointers : Error<
> "%0 attribute cannot be used with pointers to members">;
> def err_attribute_invalid_implicit_this_argument : Error<
> @@ -2181,7 +2188,7 @@
> "functions, methods and blocks|functions, methods, and classes|"
> "functions, methods, and parameters|classes|variables|methods|"
> "variables, functions and labels|fields and global variables|structs|"
> - "variables, functions and tag types|thread-local variables|"
> + "variables and tag types|thread-local variables|"
This change seems somewhat unrelated to this patch -- your attribute
doesn't appertain to tags, but typedefs. I think you may want to add a
new entry to this for "typedefs and variables" instead.
> "variables and fields|variables, data members and tag types|"
> "types and namespaces|Objective-C interfaces|methods and properties|"
> "struct or union|struct, union or class|types|"
> Index: include/clang/Sema/AttributeList.h
> ===================================================================
> --- include/clang/Sema/AttributeList.h
> +++ include/clang/Sema/AttributeList.h
> @@ -827,7 +827,7 @@
> ExpectedVariableFunctionOrLabel,
> ExpectedFieldOrGlobalVar,
> ExpectedStruct,
> - ExpectedVariableFunctionOrTag,
> + ExpectedVariableOrTag,
> ExpectedTLSVar,
> ExpectedVariableOrField,
> ExpectedVariableFieldOrTag,
> Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h
> +++ include/clang/Sema/Sema.h
> @@ -7328,6 +7328,11 @@
> void AddAlignedAttr(SourceRange AttrRange, Decl *D, TypeSourceInfo *T,
> unsigned SpellingListIndex, bool IsPackExpansion);
>
> + /// AddAlignValueAttr - Adds an align_value attribute to a particular
> + /// declaration.
> + void AddAlignValueAttr(SourceRange AttrRange, Decl *D, Expr *E,
> + unsigned SpellingListIndex);
> +
> // OpenMP directives and clauses.
> private:
> void *VarDataSharingAttributesStack;
> Index: lib/CodeGen/CGCall.cpp
> ===================================================================
> --- lib/CodeGen/CGCall.cpp
> +++ lib/CodeGen/CGCall.cpp
> @@ -1617,6 +1617,25 @@
> AI->getArgNo() + 1,
> llvm::Attribute::NonNull));
> }
> +
> + const auto *AVAttr = PVD->getAttr<AlignValueAttr>();
> + if (!AVAttr)
> + if (const auto *TOTy = dyn_cast<TypedefType>(OTy))
> + AVAttr = TOTy->getDecl()->getAttr<AlignValueAttr>();
> + if (AVAttr) {
> + llvm::Value *AlignmentValue =
> + EmitScalarExpr(AVAttr->getAlignment());
> + llvm::ConstantInt *AlignmentCI =
> + cast<llvm::ConstantInt>(AlignmentValue);
> + unsigned Alignment =
> + std::min((unsigned) AlignmentCI->getZExtValue(),
> + +llvm::Value::MaximumAlignment);
Why the +?
> +
> + llvm::AttrBuilder Attrs;
> + Attrs.addAlignmentAttr(Alignment);
> + AI->addAttr(llvm::AttributeSet::get(getLLVMContext(),
> + AI->getArgNo() + 1, Attrs));
> + }
> }
>
> if (Arg->getType().isRestrictQualified())
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -2691,6 +2691,61 @@
> Attr.getAttributeSpellingListIndex()));
> }
>
> +static void handleAlignValueAttr(Sema &S, Decl *D,
> + const AttributeList &Attr) {
Formatting?
> + S.AddAlignValueAttr(Attr.getRange(), D, Attr.getArgAsExpr(0),
> + Attr.getAttributeSpellingListIndex());
> +}
> +
> +void Sema::AddAlignValueAttr(SourceRange AttrRange, Decl *D, Expr *E,
> + unsigned SpellingListIndex) {
> + AlignValueAttr TmpAttr(AttrRange, Context, E, SpellingListIndex);
> + SourceLocation AttrLoc = AttrRange.getBegin();
> +
> + QualType T;
> + if (TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(D))
> + T = TD->getUnderlyingType();
> + else if (ValueDecl *VD = dyn_cast<ValueDecl>(D))
> + T = VD->getType();
> + else {
> + Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
> + << AttrRange << &TmpAttr /*TmpAttr.getName()*/ << ExpectedVariableOrTag;
> + return;
> + }
If you added the proper subjects to the attribute definition, you
should be able to drop the else clause here. Also the diagnostic type
is wrong (as described above). How this should be changed is to add
the new diagnostic text, update AttributeList.h properly, and then
make a quick change in ClangAttrEmitter.cpp (since I suspect typedef
or variable is a reasonable combination we may want to reuse). If
that's more work than you'd like to do, you can skip the
ClangAttrEmitter.cpp part, and manually specify the diagnostic option
in Attr.td.
> +
> + if (!T->isDependentType() && !T->isAnyPointerType() &&
> + !T->isReferenceType() && !T->isMemberPointerType()) {
> + Diag(AttrLoc, diag::warn_attribute_pointer_or_reference_only)
> + << &TmpAttr /*TmpAttr.getName()*/ << T << D->getSourceRange();
> + return;
> + }
> +
> + if (!E->isTypeDependent() && !E->isValueDependent()) {
> + llvm::APSInt Alignment(32);
> + ExprResult ICE
> + = VerifyIntegerConstantExpression(E, &Alignment,
> + diag::err_align_value_attribute_argument_not_int,
> + /*AllowFold*/ false);
> + if (ICE.isInvalid())
> + return;
> +
> + if (!Alignment.isPowerOf2()) {
> + Diag(AttrLoc, diag::err_attribute_aligned_not_power_of_two)
> + << E->getSourceRange();
> + return;
> + }
> +
> + D->addAttr(::new (Context)
> + AlignValueAttr(AttrRange, Context, ICE.get(),
> + SpellingListIndex));
> + return;
> + }
> +
> + // Save dependent expressions in the AST to be instantiated.
> + D->addAttr(::new (Context) AlignValueAttr(TmpAttr));
> + return;
> +}
> +
> static void handleAlignedAttr(Sema &S, Decl *D, const AttributeList &Attr) {
> // check the attribute arguments.
> if (Attr.getNumArgs() > 1) {
> @@ -4115,6 +4170,9 @@
> case AttributeList::AT_Aligned:
> handleAlignedAttr(S, D, Attr);
> break;
> + case AttributeList::AT_AlignValue:
> + handleAlignValueAttr(S, D, Attr);
> + break;
> case AttributeList::AT_AlwaysInline:
> handleAlwaysInlineAttr(S, D, Attr);
> break;
> Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
> ===================================================================
> --- lib/Sema/SemaTemplateInstantiateDecl.cpp
> +++ lib/Sema/SemaTemplateInstantiateDecl.cpp
> @@ -129,6 +129,17 @@
> }
> }
>
> +static void instantiateDependentAlignValueAttr(
> + Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
> + const AlignValueAttr *Aligned, Decl *New) {
> + // The alignment expression is a constant expression.
> + EnterExpressionEvaluationContext Unevaluated(S, Sema::ConstantEvaluated);
> + ExprResult Result = S.SubstExpr(Aligned->getAlignment(), TemplateArgs);
> + if (!Result.isInvalid())
> + S.AddAlignValueAttr(Aligned->getLocation(), New, Result.getAs<Expr>(),
> + Aligned->getSpellingListIndex());
Formatting?
> +}
> +
> static void instantiateDependentEnableIfAttr(
> Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
> const EnableIfAttr *A, const Decl *Tmpl, Decl *New) {
> @@ -176,6 +187,12 @@
> continue;
> }
>
> + const AlignValueAttr *AlignValue = dyn_cast<AlignValueAttr>(TmplAttr);
> + if (AlignValue && AlignValue->getAlignment()->isValueDependent()) {
> + instantiateDependentAlignValueAttr(*this, TemplateArgs, AlignValue, New);
> + continue;
> + }
> +
> const EnableIfAttr *EnableIf = dyn_cast<EnableIfAttr>(TmplAttr);
> if (EnableIf && EnableIf->getCond()->isValueDependent()) {
> instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
> Index: test/CodeGen/align_value.cpp
> ===================================================================
> --- /dev/null
> +++ test/CodeGen/align_value.cpp
> @@ -0,0 +1,8 @@
> +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
> +
> +typedef double * __attribute__((align_value(64))) aligned_double;
> +
> +void foo(aligned_double x, double * y __attribute__((align_value(32))),
> + double & z __attribute__((align_value(128)))) { };
> +// CHECK: define void @_Z3fooPdS_Rd(double* align 64 %x, double* align 32 %y, double* dereferenceable(8) align 128 %z)
> +
> Index: test/Sema/align_value.c
> ===================================================================
> --- /dev/null
> +++ test/Sema/align_value.c
> @@ -0,0 +1,29 @@
> +// RUN: %clang_cc1 -fsyntax-only -verify %s
> +
> +typedef double * __attribute__((align_value(64))) aligned_double;
> +
> +void foo(aligned_double x, double * y __attribute__((align_value(32)))) { };
> +
> +// expected-error at +1 {{requested alignment is not a power of 2}}
> +typedef double * __attribute__((align_value(63))) aligned_double1;
> +
> +// expected-error at +1 {{requested alignment is not a power of 2}}
> +typedef double * __attribute__((align_value(-2))) aligned_double2;
> +
> +// expected-error at +1 {{attribute takes one argument}}
> +typedef double * __attribute__((align_value(63, 4))) aligned_double3;
> +
> +// expected-error at +1 {{attribute takes one argument}}
> +typedef double * __attribute__((align_value())) aligned_double3a;
> +
> +// expected-error at +1 {{attribute takes one argument}}
> +typedef double * __attribute__((align_value)) aligned_double3b;
> +
> +// expected-error at +1 {{'align_value' attribute requires integer constant}}
> +typedef double * __attribute__((align_value(4.5))) aligned_double4;
> +
> +// expected-warning at +1 {{'align_value' attribute only applies to a pointer or reference ('int' is invalid)}}
> +typedef int __attribute__((align_value(32))) aligned_int;
> +
> +typedef double * __attribute__((align_value(32*2))) aligned_double5;
> +
> Index: test/SemaCXX/align_value.cpp
> ===================================================================
> --- /dev/null
> +++ test/SemaCXX/align_value.cpp
If you decide to add the CXX11 spelling, this would be a good place to
use the C++11-style syntax.
> @@ -0,0 +1,26 @@
> +// RUN: %clang_cc1 -fsyntax-only -verify %s
> +
> +typedef double * __attribute__((align_value(64))) aligned_double;
> +
> +void foo(aligned_double x, double * y __attribute__((align_value(32))),
> + double & z __attribute__((align_value(128)))) { };
> +
> +template <typename T, int Q>
> +struct x {
> + typedef T* aligned_int __attribute__((align_value(32+8*Q)));
> + aligned_int V;
> +
> + void foo(aligned_int a, T &b __attribute__((align_value(sizeof(T)*4))));
> +};
> +
> +x<float, 4> y;
> +
> +template <typename T, int Q>
> +struct nope {
> + // expected-error at +1 {{requested alignment is not a power of 2}}
> + void foo(T &b __attribute__((align_value(sizeof(T)+1))));
> +};
> +
> +// expected-note at +1 {{in instantiation of template class 'nope<long double, 4>' requested here}}
> +nope<long double, 4> y2;
> +
>
~Aaron
More information about the cfe-commits
mailing list