[PATCH] align_value attribute in Clang

Thu Jul 24 11:04:11 PDT 2014

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

>
>>
>> >
>> >>
>> >> Do we want a clang-scoped C++11 attribute for this?

Btw, this question may have been missed.

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

~Aaron