[PATCH] align_value attribute in Clang

[Aaron Ballman]

On Thu, Jul 24, 2014 at 2:58 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 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.?

Just need to add the spelling (and perhaps one of your C++ tests can
use that spelling so we don't regress on it).

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

"Good" isn't the adjective I would use here. ;-) You have an attribute
that is both a declaration attribute (it goes on specific variable
declarations and follows just that variable around), as well as a type
attribute (it goes on a typedef, but then sugars that type).

I think I would make it just inherit from Attr instead of TypeAttr,
but then model the sugaring stuff off how Regparm is handled for
AttributedType. The type attribute stuff isn't as automated as the
declaration attribute stuff (I've not gotten over there yet). You'll
need some more type-specific tests that ensure the attribute is
handled properly.

HTH!

~Aaron