[PATCH] align_value attribute in Clang

Aaron Ballman aaron.ballman at gmail.com
Thu Jul 24 09:21:10 PDT 2014


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?

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?

> +}
> +
>  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.

> +  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.

>  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 *?

> +          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.

> +}
> +
> +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.

> +  }
> +
> +  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?

> +      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?

> +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.

~Aaron




More information about the cfe-commits mailing list