[PATCH] align_value attribute in Clang
Hal Finkel
hfinkel at anl.gov
Thu Jul 24 09:32:49 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>, "Richard Smith" <richard at metafoo.co.uk>, "llvm cfe" <cfe-commits at cs.uiuc.edu>
> Sent: Thursday, July 24, 2014 11:21:10 AM
> Subject: Re: [PATCH] align_value attribute in Clang
>
> On Tue, Jul 22, 2014 at 10:09 PM, hfinkel at anl.gov <hfinkel at anl.gov>
> wrote:
> > Hi rsmith, aaron.ballman,
> >
> > This patch introduces support for the align_value attribute. This
> > attribute is supported by Intel's compiler (versions 14.0+), and
> > several of my HPC users have requested support in Clang. It
> > specifies an alignment assumption on the values to which a pointer
> > points, and is used by numerical libraries to encourage efficient
> > generation of vector code.
> >
> > Of course, we already have an aligned attribute that can specify
> > enhanced alignment for a type, so why is this additional attribute
> > important? The problem is that if you want to specify that an
> > input array of T is, say, 64-byte aligned, you could try this:
> >
> > typedef double aligned_double __attribute__((aligned(64)));
> > void foo(aligned_double *P) {
> > double x = P[0]; // This is fine.
> > double y = P[1]; // What alignment did those doubles have again?
> > }
> >
> > the access here to P[1] causes problems. P was specified as a
> > pointer to type aligned_double, and any object of type
> > aligned_double must be 64-byte aligned. But if P[0] is 64-byte
> > aligned, then P[1] cannot be, and this access causes undefined
> > behavior. Getting round this problem requires a lot of awkward
> > casting and hand-unrolling of loops, all of which is bad.
> >
> > With the align_value attribute, we can accomplish what we'd like in
> > a well defined way:
> > typedef double *aligned_double_ptr
> > __attribute__((align_value(64)));
> > void foo(aligned_double_ptr P) {
> > double x = P[0]; // This is fine.
> > double y = P[1]; // This is fine too.
> > }
> >
> > This patch is not predicated on any uncommitted LLVM features --
> > CodeGen just adds the align attribute on function arguments, which
> > has the desired effect as of r213670 -- (although, as follow-up
> > work, can be enhanced to apply to more than just function
> > parameters when combined with the in-development llvm.assume
> > functionality).
> >
> > Some documentation on the align_value attribute from Intel is on
> > this page:
> > https://software.intel.com/en-us/articles/data-alignment-to-assist-vectorization
> >
> > Thanks again!
> >
> > P.S. I would have chosen to call this aligned_value, not
> > align_value, for better naming consistency with the aligned
> > attribute, but I think it would be more useful to users to adopt
> > Intel's name.
> >
> > http://reviews.llvm.org/D4635
> >
> > Files:
> > include/clang/Basic/Attr.td
> > include/clang/Basic/AttrDocs.td
> > include/clang/Basic/DiagnosticSemaKinds.td
> > 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.cpp
>
> > Index: include/clang/Basic/Attr.td
> > ===================================================================
> > --- include/clang/Basic/Attr.td
> > +++ include/clang/Basic/Attr.td
> > @@ -354,6 +354,12 @@
> > let Documentation = [Undocumented];
> > }
> >
> > +def AlignValue : InheritableAttr {
> > + let Spellings = [GNU<"align_value">, Declspec<"align_value">];
>
> I cannot find any documentation on this being a valid Declspec
> attribute on MSDN (and VS 2013 concurs). Is this something in one of
> the newest CTPs?
No, it is supported by the Intel compiler as an extension on Windows. So it is an Intel Microsoftism in addition to being an Intel GNUism ;)
>
> Do we want a clang-scoped C++11 attribute for this?
>
> > + let Args = [ExprArgument<"Alignment">];
> > + let Documentation = [AlignValueDocs];
>
> What subjects does this attribute appertain to?
Variables and Typedefs. The other attributes that apply to things like this (like the Aligned attribute) have their Subjects line commented out. Do you know why? I can certainly add one.
>
> > +}
> > +
> > 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,24 @@
> > }];
> > }
> >
> > +def AlignValueDocs : Documentation {
> > + let Category = DocCatType;
> > + let Heading = "align_value";
>
> Shouldn't be a need to specify the heading here since the spellings
> are all identical.
Ok, thanks!
>
> > + 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) { ... }
> > + }];
> > +}
> > +
> > 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
> > @@ -1899,6 +1899,8 @@
> > "%0 attribute is not supported for this target">;
> > 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">;
>
> This should be combined with the preceding diagnostic and just use a
> %select.
I don't think that I can, the diagnostic ID gets passed into the VerifyIntegerConstantExpression utility.
>
> > 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">;
> > @@ -1934,6 +1936,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<
> > Index: include/clang/Sema/Sema.h
> > ===================================================================
> > --- include/clang/Sema/Sema.h
> > +++ include/clang/Sema/Sema.h
> > @@ -7283,6 +7283,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
> > @@ -1527,6 +1527,25 @@
> > AI->getArgNo() +
> > 1,
> > llvm::Attribute::NonNull));
> > }
> > +
> > + auto *AVAttr = PVD->getAttr<AlignValueAttr>();
>
> const auto *?
Yea, probably.
>
> > + 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));
> > + }
> > }
> >
> > if (Arg->getType().isRestrictQualified())
> > Index: lib/Sema/SemaDeclAttr.cpp
> > ===================================================================
> > --- lib/Sema/SemaDeclAttr.cpp
> > +++ lib/Sema/SemaDeclAttr.cpp
> > @@ -2679,6 +2679,62 @@
> > Attr.getAttributeSpellingListIndex()));
> > }
> >
> > +static void handleAlignValueAttr(Sema &S, Decl *D,
> > + const AttributeList &Attr) {
> > + S.AddAlignValueAttr(Attr.getRange(), D, Attr.getArgAsExpr(0),
> > + Attr.getAttributeSpellingListIndex());
> > + return;
>
> Spurious return.
I'm not sure that I'd call it 'spurious', just unnecessary :-)
>
> > +}
> > +
> > +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_attr_wrong_decl)
> > + << &TmpAttr /*TmpAttr.getName()*/ << AttrRange;
> > + return;
>
> This is usually diagnosed with warn_attribute_wrong_decl_type or
> err_attribute_wrong_decl_type.
Okay.
>
> > + }
> > +
> > + 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 &&
> > !llvm::isPowerOf2_64(Alignment.getZExtValue())) {
>
> Perhaps: Alignment.isPowerOf2() instead?
Sounds good.
>
> > + 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) {
> > @@ -4102,6 +4158,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());
> > +}
> > +
> > static void instantiateDependentEnableIfAttr(
> > Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
> > const EnableIfAttr *A, const Decl *Tmpl, Decl *New) {
> > @@ -183,6 +194,12 @@
> > continue;
> > }
> >
> > + const AlignValueAttr *AlignValue =
> > dyn_cast<AlignValueAttr>(TmplAttr);
> > + if (AlignValue &&
> > AlignValue->getAlignment()->isValueDependent()) {
> > + instantiateDependentAlignValueAttr(*this, TemplateArgs,
> > AlignValue, New);
> > + continue;
> > + }
> > +
> > assert(!TmplAttr->isPackExpansion());
> > if (TmplAttr->isLateParsed() && LateAttrs) {
> > // Late parsed attributes must be instantiated and attached
> > after the
> > 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.cpp
> > ===================================================================
> > --- /dev/null
> > +++ test/Sema/align_value.cpp
>
> Since this test is in Sema, it should be align_value.c instead.
>
> > @@ -0,0 +1,49 @@
> > +// 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)))) { };
> > +
> > +// expected-error at +1 {{requested alignment is not a power of 2}}
>
> I'm totally unfamiliar with expected-error at +1; what does that
> mean/do?
It means that the error is expected on the next line (you can use @+n to mean n lines ahead).
>
> > +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_double3;
> > +
> > +// expected-error at +1 {{attribute takes one argument}}
> > +typedef double * __attribute__((align_value)) aligned_double3;
> > +
> > +// 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;
> > +
> > +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;
> > +
>
> So this test could either move into SemaCXX, or could be split into
> two tests. If we add a C++11 attribute spelling, I think splitting
> into two tests would probably make the most sense. If we don't, then
> perhaps just move the entire test into SemaCXX.
Sounds good. I'll split it.
Thanks again,
Hal
>
> ~Aaron
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the cfe-commits
mailing list