[PATCH] align_value attribute in Clang

[Hal Finkel]

----- 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 1:04:11 PM
> Subject: Re: [PATCH] align_value attribute in Clang
> 
> On Thu, Jul 24, 2014 at 1:32 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> > ----- 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.
> 
> I would prefer to leave that spelling out at this point, then.

Okay, sure.

> The
> only way to turn that spelling on is with -fms-compatibility, and
> this
> has nothing to do with MS compatibility (in fact, it makes your code
> *less* compatible with MSVC). 

It is not clear to me that helping people write MSVC-compatible code is the purpose of MS compatibility mode, but let's have that conversation some other time. ;)

> However, if there are more instances of
> ICC attributes we would like to support in this fashion, we should
> explore a good way to expose them. For instance, the Declspec
> spelling
> could take a flag that specifies "MSVC only, ICC only, or MSVC and
> ICC" (perhaps).

Makes sense.

> 
> >
> >>
> >> >
> >> >>
> >> >> Do we want a clang-scoped C++11 attribute for this?
> 
> Btw, this question may have been missed.

It was, I apologize. Having one makes sense in my opinion. Is this just a matter of including an additional spelling in the .td file, or does more code need to be written in Sema, etc.?

> 
> >> >>
> >> >> > +  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.
> 
> Sorry, this was not meant as "don't implement this because it's
> ugly."
> It was meant as "Aligned is a poor model for most attributes because
> it has a lot of special-case machinery needed for all the various
> language modes, semantics, etc." That doesn't mean it's the wrong way
> for you to model per se, just means that it's not a particularly
> awesome example.
> 
> >> 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.
> 
> This suggests this is a declaration attribute.
> 
> >>
> >> 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.
> 
> Sorry, mistype -- meant to have "pointed to" in my question.
> 
> Okay, so then what about this:
> 
> template <typename Ty>
> void f(Ty V) { /* Does something with V */ }
> 
> typedef double * __attribute__((align_value(64)) aligned_double;
> 
> void g(aligned_double D) {
>   f(D);
> }
> 
> Is the double pointed to by V (in f()) known to be aligned within
> f()?
> Basically, I'm trying to wrap my head around whether this attribute
> follows the type or the declaration. If it follows the type, then f()
> knows that *V is 64-bit aligned. If it follows the declaration, f()
> knows nothing (though the value passed in will certainly still be
> aligned). This matters because if it follows the type, there's more
> work to be done (basically, you need to use an AttributedType, like
> address spaces, vector_size, etc). If it just follows the
> declaration,
> the documentation isn't as clear as it could be.

Ah, I see, thanks! Yes, it needs to follow the type here. So I suppose that I need to make it say:

def AlignValue : TypeAttr { ... }

Is there a good existing model for this (RegparmAttr perhaps)?

Thanks again,
Hal

> 
> ~Aaron
> 

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