[PATCH] align_value attribute in Clang
Hal Finkel
hfinkel at anl.gov
Tue Aug 26 08:35:30 PDT 2014
----- Original Message -----
> From: "Aaron Ballman" <aaron.ballman at gmail.com>
> To: reviews+D4635+public+b7e82bdc1d8c324a at reviews.llvm.org
> Cc: "Hal Finkel" <hfinkel at anl.gov>, "Michael Spencer" <bigcheesegs at gmail.com>, "Richard Smith"
> <richard at metafoo.co.uk>, "Alexey Bataev" <a.bataev at hotmail.com>, "llvm cfe" <cfe-commits at cs.uiuc.edu>,
> "nurmukhametov alex" <nurmukhametov.alex at gmail.com>
> Sent: Tuesday, August 26, 2014 9:58:50 AM
> Subject: Re: [PATCH] align_value attribute in Clang
>
> 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.
Thanks, docs got out of sync after I changes my mind about adding the C++ attribute in this patch :(
>
> > + }];
> > +}
> > +
> > 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.
Does "tag type" mean an empty struct?
>
> > "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 +?
Because it forces evaluation in order to prevent debug builds from having a link time dependency on Value.o. It appears this way everywhere in the codebase (both in Clang and in LLVM). We should probably fix this somehow, but I'm not sure how, and that's a separate matter.
>
> > +
> > + 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.
Will do. Looks like I'm missing tests for this anyway.
>
> > +
> > + 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
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the cfe-commits
mailing list