On Tue, Aug 26, 2014 at 11:35 AM, Hal Finkel <[email protected]> wrote:
> ----- Original Message -----
>> From: "Aaron Ballman" <[email protected]>
>> To: [email protected]
>> Cc: "Hal Finkel" <[email protected]>, "Michael Spencer"
>> <[email protected]>, "Richard Smith"
>> <[email protected]>, "Alexey Bataev" <[email protected]>, "llvm cfe"
>> <[email protected]>,
>> "nurmukhametov alex" <[email protected]>
>> 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, [email protected] <[email protected]>
>> 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 :(
No worries!
>
>>
>> > + }];
>> > +}
>> > +
>> > 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?
Tag type means class, struct, union or enum (corresponds to a TagDecl).
>
>>
>> > "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.
Huh, neat, I hadn't noticed that. Thank you for the education!
>
>>
>> > +
>> > + 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@+1 {{requested alignment is not a power of 2}}
>> > +typedef double * __attribute__((align_value(63))) aligned_double1;
>> > +
>> > +// expected-error@+1 {{requested alignment is not a power of 2}}
>> > +typedef double * __attribute__((align_value(-2))) aligned_double2;
>> > +
>> > +// expected-error@+1 {{attribute takes one argument}}
>> > +typedef double * __attribute__((align_value(63, 4)))
>> > aligned_double3;
>> > +
>> > +// expected-error@+1 {{attribute takes one argument}}
>> > +typedef double * __attribute__((align_value())) aligned_double3a;
>> > +
>> > +// expected-error@+1 {{attribute takes one argument}}
>> > +typedef double * __attribute__((align_value)) aligned_double3b;
>> > +
>> > +// expected-error@+1 {{'align_value' attribute requires integer
>> > constant}}
>> > +typedef double * __attribute__((align_value(4.5)))
>> > aligned_double4;
>> > +
>> > +// expected-warning@+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@+1 {{requested alignment is not a power of 2}}
>> > + void foo(T &b __attribute__((align_value(sizeof(T)+1))));
>> > +};
>> > +
>> > +// expected-note@+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
~Aaron
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
