Re: [PATCH v3] Extend the simd function attribute
Thank you Szabolcs for working on this. > On Nov 14, 2019, at 2:23 PM, Szabolcs Nagy wrote: > > Sorry v2 had a bug. > > v2: added documentation and tests. > v3: fixed expand_simd_clones so different isa variants are actually >generated. > > GCC currently supports two ways to declare the availability of vector > variants of a scalar function: > > #pragma omp declare simd > void f (void); > > and > > __attribute__ ((simd)) > void f (void); > > However these declare a set of symbols that are different simd variants > of f, so a library either provides definitions for all those symbols or > it cannot use these declarations. (The set of declared symbols can be > narrowed down with additional omp clauses, but not enough to allow > declaring a single symbol. OpenMP 5 has a declare variant feature that > allows declaring more specific simd variants, but it is complicated and > still requires gcc or vendor extension for unambiguous declarations.) > It is not just that it is complicated, it is also a good idea to make math function vectorization orthogonal to OpenMP. This is needed din clang too, thank you for shaping a solution. It would be good if we could come up with a common solution! > This patch extends the gcc specific simd attribute such that it can > specify a single vector variant of simple scalar functions (functions > that only take and return scalar integer or floating type values): > > > > where mask is "inbranch" or "notinbranch" like now, simdlen is an int > with the same meaning as in omp declare simd and simdabi is a string > specifying the call ABI (which the intel vector ABI calls ISA). The > name is optional and allows a library to use a different symbol name > than what the vector ABI specifies. > Can we have also handling also the simplest case for the use of linear (just with step = OpenMP default = 1)? It will be useful for `sincos`. OpenMP `linear` uses parameters name, in the attribute, which applies to declaration with unnamed parameters, we could use positions. Also, can we avoid making the attribute a varargs attribute, but requires all arguments to be present? We could use dummy values when the descriptor is not needed (e.g. `no_linear`). I would also require the name to be specified, with the additional requirement to make the declaration of name visible in the same compilation unit. For example, on Arm, mapping `sincos` and `exp` to unmasked vector versions with 2 lanes would be: ``` void _ZGVnN2vl8l8_sincos(float64x2_t, double *, double *); void sincos(double, double*, double *) __attribute__(simd(notinbranch,2,”simd”,_ZGVnN2vl8l8_sincos, linear(2,3))); void _ZGVnN2v_exp(float64x2_t); void exp(double) __attribute__(simd(notinbranch,2,”simd”,_ZGVnN2v_exp, no_linear)); ``` For scalable vectorization on SVE, this could be rendered as (note that SVE requires mask parameter to be present independently of the [not]inbranch clause, see [1]): ``` void _ZGVsMxvl8l8_sincos(svfloat64_t, double *, double *, svbool_t); void sincos(double, double*, double *) __attribute__(simd(inbranch,scalable,”sve”,_ZGVsMxvl8l8_sincos, linear(2,3))); void _ZGVsNxv_exp(svfloat64_t, svbool_t); void exp(double) __attribute__(simd(inbranch,scalable,”sve”,_ZGVsMxv_exp, no_linear)); ``` SVE specific note: the “scalable” token could be replaced with 0. My preference would be for using scalable as a keywords, because 0 is not allowed by OpenMP (and might also be misleading). Kind regards, Francesco [1] https://github.com/ARM-software/software-standards/blob/master/abi/vfabia64/vfabia64.rst#masking > The simd attribute currently can be used for both declarations and > definitions, in the latter case the simd varaints of the function are > generated, which should work with the extended simd attribute too. > > Tested on aarch64-linux-gnu and x86_64-linux-gnu. > > gcc/ChangeLog: > > 2019-11-14 Szabolcs Nagy > > * cgraph.h (struct cgraph_simd_clone): Add simdname field. > * doc/extend.texi: Update the simd attribute documentation. > * tree.h (OMP_CLAUSE__SIMDABI__EXPR): Define. > (OMP_CLAUSE__SIMDNAME__EXPR): Define. > * tree.c (walk_tree_1): Handle new omp clauses. > * tree-core.h (enum omp_clause_code): Likewise. > * tree-nested.c (convert_nonlocal_omp_clauses): Likewise. > * tree-pretty-print.c (dump_omp_clause): Likewise. > * omp-low.c (scan_sharing_clauses): Likewise. > * omp-simd-clone.c (simd_clone_clauses_extract): Likewise. > (simd_clone_mangle): Handle simdname. > (expand_simd_clones): Reset vecsize_mangle when generating clones. > * config/aarch64/aarch64.c > (aarch64_simd_clone_compute_vecsize_and_simdlen): Warn about > unsupported SIMD ABI. > * config/i386/i386.c > (ix86_simd_clone_compute_vecsize_and_simdlen): Likewise. > > gcc/c-family/ChangeLog: > > 2019-11-14 Szabolcs Nagy > > * c-attribs.c (handle_simd_attribute): Handle
Re: [PATCH v3] Extend the simd function attribute
> On Nov 15, 2019, at 10:01 AM, Szabolcs Nagy wrote:
>
> On 15/11/2019 15:05, Francesco Petrogalli wrote:
>> Thank you Szabolcs for working on this.
>>
>>> On Nov 14, 2019, at 2:23 PM, Szabolcs Nagy wrote:
>>>
>>> Sorry v2 had a bug.
>>>
>>> v2: added documentation and tests.
>>> v3: fixed expand_simd_clones so different isa variants are actually
>>> generated.
>>>
>>> GCC currently supports two ways to declare the availability of vector
>>> variants of a scalar function:
>>>
>>> #pragma omp declare simd
>>> void f (void);
>>>
>>> and
>>>
>>> __attribute__ ((simd))
>>> void f (void);
>>>
>>> However these declare a set of symbols that are different simd variants
>>> of f, so a library either provides definitions for all those symbols or
>>> it cannot use these declarations. (The set of declared symbols can be
>>> narrowed down with additional omp clauses, but not enough to allow
>>> declaring a single symbol. OpenMP 5 has a declare variant feature that
>>> allows declaring more specific simd variants, but it is complicated and
>>> still requires gcc or vendor extension for unambiguous declarations.)
>>>
>>
>> It is not just that it is complicated, it is also a good idea to make math
>> function vectorization orthogonal to OpenMP.
>> This is needed din clang too, thank you for shaping a solution. It would be
>> good if we could come up with a common solution!
>>
>>> This patch extends the gcc specific simd attribute such that it can
>>> specify a single vector variant of simple scalar functions (functions
>>> that only take and return scalar integer or floating type values):
>>>
>>>
>>>
>>> where mask is "inbranch" or "notinbranch" like now, simdlen is an int
>>> with the same meaning as in omp declare simd and simdabi is a string
>>> specifying the call ABI (which the intel vector ABI calls ISA). The
>>> name is optional and allows a library to use a different symbol name
>>> than what the vector ABI specifies.
>>>
>>
>> Can we have also handling also the simplest case for the use of linear (just
>> with step = OpenMP default = 1)? It will be useful for `sincos`.
>>
>> OpenMP `linear` uses parameters name, in the attribute, which applies to
>> declaration with unnamed parameters, we could use positions.
>>
>> Also, can we avoid making the attribute a varargs attribute, but requires
>> all arguments to be present? We could use dummy values when the descriptor
>> is not needed (e.g. `no_linear`).
>>
>> I would also require the name to be specified, with the additional
>> requirement to make the declaration of name visible in the same compilation
>> unit.
>>
>> For example, on Arm, mapping `sincos` and `exp` to unmasked vector versions
>> with 2 lanes would be:
>>
>> ```
>> void _ZGVnN2vl8l8_sincos(float64x2_t, double *, double *);
>>
>> void sincos(double, double*, double *)
>> __attribute__(simd(notinbranch,2,”simd”,_ZGVnN2vl8l8_sincos, linear(2,3)));
>>
>> void _ZGVnN2v_exp(float64x2_t);
>>
>> void exp(double) __attribute__(simd(notinbranch,2,”simd”,_ZGVnN2v_exp,
>> no_linear));
>> ```
>
>
> note that exp returns double.
>
Facepalm myself. Sorry.
The examples still holds though, replacing `void` with `double` for `exp`.
> the simdabi is "n" instead of "simd", it is the
> 'ISA character' from the vector abi, to make it clear
> how it is tied to the vector abi name mangling.
>
Good choice, I like it more.
> pre-declaration of the simd symbol is not required,
> since the current simd attr (and omp pragma) works
> that way (e.g. it means simd types need not be visible
> where it is used, which may be tricky if they need
> target specific header inclusion).
>
Fair enough.
> notinbranch is a string in quotes, to avoid macro
> name collision in standard headers.
>
Fine by me.
> the same issue applies to linear, so if we add such
> argument it must not use bare 'linear' identifier, e.g.
> it could be an int list like
>
> simd("notinbranch", 2, "n", {2,3}, "vsincos")
>
I like it. Can it be an empty `{}` when no linear parameters are available?
> or we could just specify the vector abi name and
> reverse engineer the requirements from that:
>
> simd("", "_ZGVnN2vl8l8_sincos", "vsincos&qu
Re: [PATCH v3] Extend the simd function attribute
On 11/20/19 7:54 AM, Szabolcs Nagy wrote:
> On 14/11/2019 20:23, Szabolcs Nagy wrote:
>> Sorry v2 had a bug.
>>
>> v2: added documentation and tests.
>> v3: fixed expand_simd_clones so different isa variants are actually
>> generated.
>>
>> GCC currently supports two ways to declare the availability of vector
>> variants of a scalar function:
>>
>>#pragma omp declare simd
>>void f (void);
>>
>> and
>>
>>__attribute__ ((simd))
>>void f (void);
>>
>> However these declare a set of symbols that are different simd variants
>> of f, so a library either provides definitions for all those symbols or
>> it cannot use these declarations. (The set of declared symbols can be
>> narrowed down with additional omp clauses, but not enough to allow
>> declaring a single symbol. OpenMP 5 has a declare variant feature that
>> allows declaring more specific simd variants, but it is complicated and
>> still requires gcc or vendor extension for unambiguous declarations.)
>>
>> This patch extends the gcc specific simd attribute such that it can
>> specify a single vector variant of simple scalar functions (functions
>> that only take and return scalar integer or floating type values):
>>
>>__attribute__ ((simd (mask, simdlen, simdabi, name
> ping.
>
> so the comments so far
>
> - make all attribute arguments mandatory (e.g don't allow
>simd(mask, simdlen)), this is fine with me if others agree.
works for me :)
> - support the linear clause for pointer arguments (sincos).
>this requires listing arguments to which linear applies,
>i would only want to do that if there is a hope that it
>will ever be useful (currently gcc won't vectorize calls
>with pointer arguments, but maybe it should?).
If the C attribute inherit the properties of `declare simd` (or the
`variant` equivalent), it means that the function can be invoked
concurrently. This to me is enough to say that the following loop is
vectorizable (provided that the presence of vector sincos is the only
condition that prevents the loop from vectorizing)
void sincos(double , double *, double *) __attribute__((simd(noinbranch,
2, {1,2} /*linear*/, "n", "_ZGVnN2vl8l8_sincos"));
...
for (int i=...)
sincos(in[i], &sin[i], &cos[i]);
So overall, yes, I think a compiler should vectorize this example.
Please let me know if I am missing anything.
Side question: what would be the behavior of the attribute when attached
to a function definition? Are you expecting the compiler to
auto-vectorize the function? Given that the attribute is needed only for
interfacing libraries, I wouldn't recommend to use to auto-vectorize
functions. I am asking because I think you mentioned that the attribute
mimics the OpenMP clause...
> i don't know
>of a precedent for "list of integers" used in the attribute
>syntax, so i wonder what's the right way to do it.
>
> - plain simd should have fixed abi for a given architecture:
>aarch64 can of course do this, but if we include sve, then
>libmvec with plain simd attr won't be testable with gcc-10
>since gcc-10 does not support simd attr for sve, so we still
>need the attribute extension to do work on libmvec.
>
> any more comments on supporting linear clause in simd attr?
> or if the posted patch is reasonable as is?
Hum, I can't find the patch update in your last message...
>> where mask is "inbranch" or "notinbranch" like now, simdlen is an int
>> with the same meaning as in omp declare simd and simdabi is a string
>> specifying the call ABI (which the intel vector ABI calls ISA). The
>> name is optional and allows a library to use a different symbol name
>> than what the vector ABI specifies.
>>
>> The simd attribute currently can be used for both declarations and
>> definitions, in the latter case the simd varaints of the function are
>> generated, which should work with the extended simd attribute too.
>>
>> Tested on aarch64-linux-gnu and x86_64-linux-gnu.
>>
>> gcc/ChangeLog:
>>
>> 2019-11-14 Szabolcs Nagy
>>
>> * cgraph.h (struct cgraph_simd_clone): Add simdname field.
>> * doc/extend.texi: Update the simd attribute documentation.
>> * tree.h (OMP_CLAUSE__SIMDABI__EXPR): Define.
>> (OMP_CLAUSE__SIMDNAME__EXPR): Define.
>> * tree.c (walk_tree_1): Handle new omp clauses.
>> * tree-core.h (enum omp_clause_code): Likewise.
>> * tree-nested.c (convert_nonlocal_omp_clauses): Likewise.
>> * tree-pretty-print.c (dump_omp_clause): Likewise.
>> * omp-low.c (scan_sharing_clauses): Likewise.
>> * omp-simd-clone.c (simd_clone_clauses_extract): Likewise.
>> (simd_clone_mangle): Handle simdname.
>> (expand_simd_clones): Reset vecsize_mangle when generating clones.
>> * config/aarch64/aarch64.c
>> (aarch64_simd_clone_compute_vecsize_and_simdlen): Warn about
>> unsupported SIMD ABI.
>> * config/i386/i386.c
>> (ix86_simd_clone_compute_vecsize_and_simdlen): Likewise.
>>
>> gcc/c-famil
Re: [PATCH v3] Extend the simd function attribute
> On Nov 21, 2019, at 4:09 PM, Richard Sandiford
> wrote:
>
> This probably isn't helpful, sorry, but the fact that the second point
> is a (reasonable) area of debate IMO shows the problems with doing
> the first point. If we go for (1) but not (2), everyone will need
> to specify the four initial parameters. But we'd have to allow (2)
> to be optional if we added it later. We might in future also want other
> parameters, e.g. for uniform arguments, by-value vs. by-ref, etc.
>
> Maybe we could have key-value pairs?
>
> __attribute__((__simd__("simdlen", 2, "simdabi", "n")))
>
> This would also allow linear parameters to be specified like this:
>
> __attribute__((__simd__("simdlen", 2, "linear", 2, "linear", 3, ...)))
>
> The current attribute syntax doesn't allow { 1, 2 } as an attribute
> parameter, and adding it now wouldn't be backward compatible (since
> older toolchains should be free to ignore this).
>
If we cannot do { 1, 2 }, I’d rather have the following syntax
__attribute__((__simd__(“name”, [not]inbranch, simdlen(2), simdabi(“n”),
linear(ref(1)[:step], uval(2)[:step],…), linear_pos(ref(1)[:pos],
uval(2)[:pos],…), uniform(4,5…), align(8,9[:val]
With no_linear, no_linear_pos, no_uniform and no_align to be specified when not
used, and step/pos/val with the same default values as in the OpenMP Standard
when not specified.
With this, all that we need to implement (for now) for sincos is (just use
no_linear for sin/exp/log...)
__attribute__((__simd__(“name”, [not]inbranch, simdlen(2), simdabi(“n”),
linear(1,2), no_linear_pos, no_uniform, no_align
(Btw, happy for all those values to be strings if we cannot use identifiers)
__attribute__((__simd__(“name”, "[not]inbranch", "simdlen(2)", "simdabi(n)",
"linear(1,2)", “no_linear_pos", “no_uniform", “no_align"
> I realise the key-value approach makes things more complicated though.
> I guess we're in danger of reinventing omp declare variant that way. :-)
>
Yep :)
How much this is gonna be useful beyond “flat vectorization” and sincos
vectorization is debatable (users can use OpenMP for this...), but at least we
would have some ground for expanding it if / when needed, with backward
compatibility in mind.
> I'm in two minds about whether we should expose the simdabi as the
> mangling character. On the one hand it's simple, on the other it seems
> a bit too low-level for something like this. I think we'd need to check
> whether the parameter is valid anyway, so maybe we should have some more
> user-friendly mnemonics?
>
No preference here, if not for the fact that the mangling character is already
encoded in the specs, so there is no space for interpretation: all we need to
do is to refer to the Vector Function ABIs.
Thanks!
Francesco
