[PATCH] align_value attribute in Clang

Hal Finkel hfinkel at anl.gov
Thu Jul 24 10:32:10 PDT 2014


----- Original Message -----
> From: "Aaron Ballman" <aaron.ballman at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Richard Smith" <richard at metafoo.co.uk>, "llvm cfe" <cfe-commits at cs.uiuc.edu>,
> reviews+D4635+public+b7e82bdc1d8c324a at reviews.llvm.org
> Sent: Thursday, July 24, 2014 12:04:30 PM
> Subject: Re: [PATCH] align_value attribute in Clang
> 
> 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.

Eeh... We could. I don't want to get too distracted by this point. Intel is obviously comfortable supporting it in their MS mode, and it is an optimization hint (so there is no harm in ignoring it). If our goal for the MS compatibility mode is to allow people to do their MS-compiler-compatible C++ windows development using Clang, then I see no reason not to include Intel extensions that they support in MS mode, as there are certainly a fair number of icc/Windows users.

Regardless, I'm not invested in the answer to this question, and have no particular interest in trying to create an ICC-compatibility mode. In short, I'd either like to just include the MS spelling or leave it out. At most, we could add a flag that enable support for this feature, but I don't want to do something that claims ICC compatibility generally.

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

Yes, and I have many users who care immensely about efficient vectorization, and thus they need to.

> Is this a type attribute, or a declaration attribute? Eg) should this
> information be following the type? What happens with:

It should follow a typedef, and also be applicable to variables.

> 
> typedef double * __attribute__((align_value(64)) aligned_double;
> std::vector<aligned_double> doubles;
> 
> Is this meant to align the doubles in the vector?

No, you did not put doubles in your vector, you put double* in your vector, and we assume that all of those pointers point to doubles that are 64-byte aligned. This is a perfectly valid use case.

Thanks again,
Hal

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

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory



More information about the cfe-commits mailing list