[PATCH] align_value attribute in Clang
Hal Finkel
hfinkel at anl.gov
Thu Oct 2 14:33:50 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 at gmail.com>
> Sent: Thursday, October 2, 2014 4:04:07 PM
> Subject: Re: [PATCH] align_value attribute in Clang
>
> On Thu, Oct 2, 2014 at 4:10 PM, hfinkel at anl.gov <hfinkel at anl.gov>
> wrote:
> > Alright, after much discussion, I think this is ready.
> >
> > To quickly recap, there had been some discussion regarding whether
> > or not align_value needed to be part of the type system (so that
> > it would be propagated by template type deduction, for example),
> > after after receiving feedback from Richard, Alexey, et al., we're
> > settled on no. This is not part of the type system, and will only
> > "propagate" through templates, auto, etc. by optimizer deduction
> > after inlining. This seems consistent with Intel's implementation.
> >
> > Aaron, this new revision should account for all additional points
> > from your last review.
>
> Aside from some very minor formatting nits, LGTM!
>
> > Index: include/clang/Basic/Attr.td
> > ===================================================================
> > --- include/clang/Basic/Attr.td
> > +++ include/clang/Basic/Attr.td
> > @@ -354,6 +354,25 @@
> > 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">
> > + ];
>
> This formatting is unlike anything else in the file. Would be good to
> tighten it up a bit by shifting to be indented under the let.
I adjusted this to what I think you meant. Please feel free to correct it, or have me do so, if it is still quite what you'd like. r218910, thanks!
-Hal
>
> > + let Args = [ExprArgument<"Alignment">];
> > + let Subjects = SubjectList<[Var, TypedefName], WarnDiag,
> > + "ExpectedVariableOrTypedef">;
> > + let Documentation = [AlignValueDocs];
> > +}
> > +
> > 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
> > @@ -1011,6 +1011,26 @@
> > }];
> > }
> >
> > +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) { ... }
> > +
> > +If the pointer value does not have the specified alignment at
> > runtime, the
> > +behavior of the program is undefined.
> > + }];
> > +}
> > +
> > 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
> > @@ -1930,8 +1930,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">;
> > @@ -1970,6 +1974,9 @@
> > def warn_attribute_return_pointers_refs_only : Warning<
> > "%0 attribute only applies to return values that are pointers or
> > references">,
> > 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<
> > @@ -2210,7 +2217,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 typedefs|thread-local variables|"
> > "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,
> > + ExpectedVariableOrTypedef,
> > ExpectedTLSVar,
> > ExpectedVariableOrField,
> > ExpectedVariableFieldOrTag,
> > Index: include/clang/Sema/Sema.h
> > ===================================================================
> > --- include/clang/Sema/Sema.h
> > +++ include/clang/Sema/Sema.h
> > @@ -7363,6 +7363,11 @@
> > void AddAssumeAlignedAttr(SourceRange AttrRange, Decl *D, Expr
> > *E, Expr *OE,
> > unsigned SpellingListIndex);
> >
> > + /// 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
> > @@ -1743,6 +1743,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);
> > +
> > + llvm::AttrBuilder Attrs;
> > + Attrs.addAlignmentAttr(Alignment);
> > + AI->addAttr(llvm::AttributeSet::get(getLLVMContext(),
> > + AI->getArgNo() +
> > 1, Attrs));
>
> Formatting.
>
> > + }
> > }
> >
> > if (Arg->getType().isRestrictQualified())
> > Index: lib/Sema/SemaDeclAttr.cpp
> > ===================================================================
> > --- lib/Sema/SemaDeclAttr.cpp
> > +++ lib/Sema/SemaDeclAttr.cpp
> > @@ -2757,6 +2757,58 @@
> > Attr.getAttributeSpellingListIndex()));
> > }
> >
> > +static void handleAlignValueAttr(Sema &S, Decl *D,
> > + const AttributeList &Attr) {
> > + 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
> > + llvm_unreachable("Unknown decl type for align_value");
> > +
> > + 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->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_alignment_not_power_of_two)
> > + << E->getSourceRange();
> > + return;
> > + }
> > +
> > + D->addAttr(::new (Context)
> > + AlignValueAttr(AttrRange, Context, ICE.get(),
> > + SpellingListIndex));
>
> Formatting.
>
> > + 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) {
> > @@ -4181,6 +4233,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
> > @@ -152,6 +152,17 @@
> > Aligned->getSpellingListIndex());
> > }
> >
> > +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());
> > +}
> > +
> > static void instantiateDependentEnableIfAttr(
> > Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
> > const EnableIfAttr *A, const Decl *Tmpl, Decl *New) {
> > @@ -205,6 +216,12 @@
> > continue;
> > }
> >
> > + const AlignValueAttr *AlignValue =
> > dyn_cast<AlignValueAttr>(TmplAttr);
> > + if (AlignValue) {
> > + 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,32 @@
> > +// 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;
> > +
> > +// expected-warning at +1 {{'align_value' attribute only applies to
> > variables and typedefs}}
> > +void foo() __attribute__((align_value(32)));
> > +
> > Index: test/SemaCXX/align_value.cpp
> > ===================================================================
> > --- /dev/null
> > +++ test/SemaCXX/align_value.cpp
> > @@ -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