[PATCH] align_value attribute in Clang
Aaron Ballman
aaron.ballman at gmail.com
Thu Jul 24 10:04:30 PDT 2014
On Thu, Jul 24, 2014 at 12:32 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- 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 need an -ficc-compatibility mode? I really don't like the fact
that this spelling will work with -fms-compatibility when this is not
a Microsoft extension.
>
>>
>> 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.
Aligned is a very nasty attribute to model after unless you have to.
Is this a type attribute, or a declaration attribute? Eg) should this
information be following the type? What happens with:
typedef double * __attribute__((align_value(64)) aligned_double;
std::vector<aligned_double> doubles;
Is this meant to align the doubles in the vector?
>>
>> > +}
>> > +
>> > 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.
Ahh, that explains that. Thanks!
>
>>
>> > 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).
You learn something new every day. :-) Thank you for the explanation.
We don't typically use this; is there a problem with simply moving the
expected diagnostic comment to the line where the diagnostic will
appear?
>
>>
>> > +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
~Aaron
More information about the cfe-commits
mailing list