Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Joseph Myers]

On Wed, 13 Mar 2019, Richard Biener wrote:

> And that fortran support patch would need yet another iteration.

Fortran never uses the _finite names because it never uses the C header 
that can declare the functions to use those asm names.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Steve Ellcey]

On Wed, 2019-03-13 at 14:38 +, Joseph Myers wrote:
> 
> ---
> ---
> On Wed, 13 Mar 2019, Jakub Jelinek wrote:
> 
> > If the finite only doesn't buy anything, then another option is to drop the
> > math-finite.h stuff or portions thereof.
> 
> Well, finite-only entry points avoid wrappers for the scalar functions - 
> it's just that suitable optimized implementations could avoid the wrappers 
> in all cases without needing a separate finite-only variant.  It's not 
> clear that adding wrappers in this case for scalar functions to avoid them 
> for vector functions is a good idea.  And regardless of the merits of a 
> particular set of entry points, I think requiring the same set of variants 
> for both vector and scalar functions is flawed; the headers should be able 
> to declare a scalar variant to be used under certain conditions without 
> requiring a corresponding vector variant, or of course the other way 
> round.

If some targets don't need _finite variants it might be useful to tell
the compiler not to generate them.  For example, on aarch64 we know
that __exp_finite and __expf_finite are just aliases of exp and expf,
so if we could turn off the math-finite.h functionality for those
functions on this target, GCC would also not generate calls to the
vector versions of exp_finite and expf_finite.  This doesn't directly
address the issue of the scalar and vector variants being independent
of each other but it does make the problem moot in this specific case.

Steve Ellcey
sell...@marvell.com

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Joseph Myers]

On Wed, 13 Mar 2019, Jakub Jelinek wrote:

> If the finite only doesn't buy anything, then another option is to drop the
> math-finite.h stuff or portions thereof.

Well, finite-only entry points avoid wrappers for the scalar functions - 
it's just that suitable optimized implementations could avoid the wrappers 
in all cases without needing a separate finite-only variant.  It's not 
clear that adding wrappers in this case for scalar functions to avoid them 
for vector functions is a good idea.  And regardless of the merits of a 
particular set of entry points, I think requiring the same set of variants 
for both vector and scalar functions is flawed; the headers should be able 
to declare a scalar variant to be used under certain conditions without 
requiring a corresponding vector variant, or of course the other way 
round.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Jakub Jelinek]

On Wed, Mar 13, 2019 at 02:07:29PM +0100, Jakub Jelinek wrote:
> On Wed, Mar 13, 2019 at 01:00:58PM +, Joseph Myers wrote:
> > > Yeah, an alias doesn't really cost much, and has the advantage that if in
> > > the vector version you need at some point to differentiate between the
> > > finite only vs. full implementations, you can just by tweaking libmvec
> > > implementation, the callers will have proper calls depending on if they 
> > > were
> > > compiled with -Ofast or -O3 etc.
> > 
> > Experience is showing that some or all of the finite-only versions in 
> > glibc were mistaken premature optimization - that proper optimized 
> > implementations do not gain anything from adding a finite-only 
> > restriction.  There is no good basis to suppose that if additional 
> > variants of the vector functions were useful in future, finite-only would 
> > be the right conditional (or that the right set of variants would be the 
> > same as the right set of variants for scalar functions).
> 
> If the finite only doesn't buy anything, then another option is to drop the
> math-finite.h stuff or portions thereof.
> But adding a new GCC extension that other compilers will need to implement
> too, instead of just adding a couple of aliases seems to be overkill to me.

Not to mention that fixing it on the glibc side will make it work also with
GCC 8, GCC 7 and GCC 6.

Jakub

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Jakub Jelinek]

On Wed, Mar 13, 2019 at 01:00:58PM +, Joseph Myers wrote:
> > Yeah, an alias doesn't really cost much, and has the advantage that if in
> > the vector version you need at some point to differentiate between the
> > finite only vs. full implementations, you can just by tweaking libmvec
> > implementation, the callers will have proper calls depending on if they were
> > compiled with -Ofast or -O3 etc.
> 
> Experience is showing that some or all of the finite-only versions in 
> glibc were mistaken premature optimization - that proper optimized 
> implementations do not gain anything from adding a finite-only 
> restriction.  There is no good basis to suppose that if additional 
> variants of the vector functions were useful in future, finite-only would 
> be the right conditional (or that the right set of variants would be the 
> same as the right set of variants for scalar functions).

If the finite only doesn't buy anything, then another option is to drop the
math-finite.h stuff or portions thereof.
But adding a new GCC extension that other compilers will need to implement
too, instead of just adding a couple of aliases seems to be overkill to me.

Jakub

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Joseph Myers]

On Wed, 13 Mar 2019, Jakub Jelinek wrote:

> Yeah, an alias doesn't really cost much, and has the advantage that if in
> the vector version you need at some point to differentiate between the
> finite only vs. full implementations, you can just by tweaking libmvec
> implementation, the callers will have proper calls depending on if they were
> compiled with -Ofast or -O3 etc.

Experience is showing that some or all of the finite-only versions in 
glibc were mistaken premature optimization - that proper optimized 
implementations do not gain anything from adding a finite-only 
restriction.  There is no good basis to suppose that if additional 
variants of the vector functions were useful in future, finite-only would 
be the right conditional (or that the right set of variants would be the 
same as the right set of variants for scalar functions).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Jakub Jelinek]

On Wed, Mar 13, 2019 at 01:01:54PM +0100, Richard Biener wrote:
> On Wed, Mar 13, 2019 at 12:40 AM Jakub Jelinek  wrote:
> >
> > On Tue, Mar 12, 2019 at 11:21:26PM +, Steve Ellcey wrote:
> > > I like this idea.  I have prototyped something, I created 'vector_asm'
> > > as an aarch64 attribute because I couldn't find where to put global
> > > attributes like __simd__.  Does anyone know where these are listed?
> >
> > gcc/c-family/c-attribs.c
> >
> > Note, vector_asm seems like a bad name when the attribute is simd
> > or directive #pragma omp declare simd, it should be simd_asm instead.
> >
> > If we go this route, glibc headers would need to use it conditional on gcc
> > version though, because older gccs wouldn't support that attribute.
> 
> And that fortran support patch would need yet another iteration.
> 
> That said, I dislike it and I do not see why a solution in glibc needs to be
> inefficient either.

Yeah, an alias doesn't really cost much, and has the advantage that if in
the vector version you need at some point to differentiate between the
finite only vs. full implementations, you can just by tweaking libmvec
implementation, the callers will have proper calls depending on if they were
compiled with -Ofast or -O3 etc.

Jakub

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Richard Biener]

On Wed, Mar 13, 2019 at 12:40 AM Jakub Jelinek  wrote:
>
> On Tue, Mar 12, 2019 at 11:21:26PM +, Steve Ellcey wrote:
> > I like this idea.  I have prototyped something, I created 'vector_asm'
> > as an aarch64 attribute because I couldn't find where to put global
> > attributes like __simd__.  Does anyone know where these are listed?
>
> gcc/c-family/c-attribs.c
>
> Note, vector_asm seems like a bad name when the attribute is simd
> or directive #pragma omp declare simd, it should be simd_asm instead.
>
> If we go this route, glibc headers would need to use it conditional on gcc
> version though, because older gccs wouldn't support that attribute.

And that fortran support patch would need yet another iteration.

That said, I dislike it and I do not see why a solution in glibc needs to be
inefficient either.

Richard.

> Jakub

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Jakub Jelinek]

On Tue, Mar 12, 2019 at 11:21:26PM +, Steve Ellcey wrote:
> I like this idea.  I have prototyped something, I created 'vector_asm'
> as an aarch64 attribute because I couldn't find where to put global
> attributes like __simd__.  Does anyone know where these are listed?

gcc/c-family/c-attribs.c

Note, vector_asm seems like a bad name when the attribute is simd
or directive #pragma omp declare simd, it should be simd_asm instead.

If we go this route, glibc headers would need to use it conditional on gcc
version though, because older gccs wouldn't support that attribute.

Jakub

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Steve Ellcey]

On Tue, 2019-03-12 at 14:17 +, Joseph Myers wrote:
> 
> On Tue, 12 Mar 2019, Richard Biener wrote:
> 
> > It shouldn't be difficult to provide an alias from the glibc side, no?  
> > How does x86 avoid this issue?
> 
> There are static-only wrappers in libmvec_nonshared.a to work around the 
> GCC limitation (not included in the shared library, so less efficient than 
> direct calls to the vectorized functions, because it ought not be 
> necessary to have multiple symbols for the same function exported from the 
> shared library for this purpose).
> 
> The issue is as I said in 
>  - vector and scalar 
> versions of functions should not need to be in one-to-one correspondence.  
> For example, you could add __attribute__ ((__vector_asm__ ("name"))) to 
> set the version of a function's name to be used as the basis for forming 
> vector function names, overriding the use of a name specified with asm 
> ("name") for that purpose.

I like this idea.  I have prototyped something, I created 'vector_asm'
as an aarch64 attribute because I couldn't find where to put global
attributes like __simd__.  Does anyone know where these are listed?
I left off the leading/trailing underscores because GCC didn't like
them in a target attribute.

I then updated my sysdeps/aarch64/fpu/bits/math-vector.h file in glibc
and things seemed to work fine.  Here are my changes, what do people
think about changing GCC to do this?

The GCC changes I made are:


diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index b38505b..45fde16 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1180,8 +1180,9 @@ static const struct attribute_spec 
aarch64_attribute_table[] =
 {
   /* { name, min_len, max_len, decl_req, type_req, fn_type_req,
affects_type_identity, handler, exclude } */
-  { "aarch64_vector_pcs", 0, 0, false, true,  true,  true,  NULL, NULL },
-  { NULL, 0, 0, false, false, false, false, NULL, NULL }
+  { "aarch64_vector_pcs", 0,  0, false, true,  true,  true,  NULL, NULL },
+  { "vector_asm", 1,  1, true,  false, false, false, NULL, NULL },
+  { NULL, 0,  0, false, false, false, false, NULL, NULL }
 };
 
 #define AARCH64_CPU_DEFAULT_FLAGS ((selected_cpu) ? selected_cpu->flags : 0)
diff --git a/gcc/omp-simd-clone.c b/gcc/omp-simd-clone.c
index 388198b..59183f0 100644
--- a/gcc/omp-simd-clone.c
+++ b/gcc/omp-simd-clone.c
@@ -342,6 +342,8 @@ simd_clone_mangle (struct cgraph_node *node,
   unsigned int simdlen = clone_info->simdlen;
   unsigned int n;
   pretty_printer pp;
+  tree attr;
+  const char *str;
 
   gcc_assert (vecsize_mangle && simdlen);
 
@@ -409,7 +411,12 @@ simd_clone_mangle (struct cgraph_node *node,
 }
 
   pp_underscore ();
-  const char *str = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (node->decl));
+  attr = lookup_attribute ("vector_asm", DECL_ATTRIBUTES (node->decl));
+  if (attr)
+str = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr)));
+  else
+str = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (node->decl));
+
   if (*str == '*')
 ++str;
   pp_string (, str);



The new sysdeps/aarch64/fpu/bits/math-vector.h was modified to include:

# ifdef __DECL_SIMD_AARCH64
#  undef __DECL_SIMD_exp
#  define __DECL_SIMD_exp __DECL_SIMD_AARCH64 __attribute__ ((vector_asm 
("exp")))
#  undef __DECL_SIMD_expf
#  define __DECL_SIMD_expf __DECL_SIMD_AARCH64 __attribute__ ((vector_asm 
("expf")))

# endif

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Joseph Myers]

On Tue, 12 Mar 2019, Richard Biener wrote:

> It shouldn't be difficult to provide an alias from the glibc side, no?  
> How does x86 avoid this issue?

There are static-only wrappers in libmvec_nonshared.a to work around the 
GCC limitation (not included in the shared library, so less efficient than 
direct calls to the vectorized functions, because it ought not be 
necessary to have multiple symbols for the same function exported from the 
shared library for this purpose).

The issue is as I said in 
 - vector and scalar 
versions of functions should not need to be in one-to-one correspondence.  
For example, you could add __attribute__ ((__vector_asm__ ("name"))) to 
set the version of a function's name to be used as the basis for forming 
vector function names, overriding the use of a name specified with asm 
("name") for that purpose.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Jakub Jelinek]

On Tue, Mar 12, 2019 at 09:33:40AM +0100, Jakub Jelinek wrote:
> On Tue, Mar 12, 2019 at 09:25:35AM +0100, Richard Biener wrote:
> > I think this needs to be fixed on the glibc side - if glibc advertises
> > 
> > float expf (float, float)
> > __attribute__((simd(notinbranch),alias("__expf_finite"))
> > 
> > or so then you of course have to provide an implementation that matches 
> > this.
> > 
> > It shouldn't be difficult to provide an alias from the glibc side, no?  How 
> > does
> > x86 avoid this issue?
> 
> Yeah, the patch looks just wrong, it is completely intentional that we use
> DECL_ASSEMBLER_NAME, it is part of the OpenMP ABI, e.g. for C++ we really
> need to use the C++ mangled names there, can't use foo or __ct instead of
> say _Z3fooii or _ZN1AC2Ev as the base parts of the names.
> 
> Please have a look at how this works on x86_64 on the glibc side and do the
> aarch64 glibc patch similarly.

Trying:
#include 

double a[1024];

void
foo (void)
{
  for (int i = 0; i < 1024; i++)
a[i] = exp (a[i]);
}

void
bar (void)
{
  for (int i = 0; i < 1024; i++)
a[i] = sin (a[i]);
}

int
main ()
{
  return 0;
}

on x86_64 with -Ofast -lmvec this works for sin, but doesn't link for exp.
You've hit a glibc bug then, this needs to be fixed on the glibc side,
probably by adding aliases.

Jakub

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Jakub Jelinek]

On Tue, Mar 12, 2019 at 09:25:35AM +0100, Richard Biener wrote:
> I think this needs to be fixed on the glibc side - if glibc advertises
> 
> float expf (float, float)
> __attribute__((simd(notinbranch),alias("__expf_finite"))
> 
> or so then you of course have to provide an implementation that matches this.
> 
> It shouldn't be difficult to provide an alias from the glibc side, no?  How 
> does
> x86 avoid this issue?

Yeah, the patch looks just wrong, it is completely intentional that we use
DECL_ASSEMBLER_NAME, it is part of the OpenMP ABI, e.g. for C++ we really
need to use the C++ mangled names there, can't use foo or __ct instead of
say _Z3fooii or _ZN1AC2Ev as the base parts of the names.

Please have a look at how this works on x86_64 on the glibc side and do the
aarch64 glibc patch similarly.

> > 2018-03-11  Steve Ellcey  
> >
> > * config/aarch64/aarch64.c (aarch64_simd_clone_vec_base_name):
> > New function.
> > (TARGET_SIMD_CLONE_VEC_BASE_NAME): New macro.
> > * doc/tm.texi.in (TARGET_SIMD_CLONE_VEC_BASE_NAME): New hook.
> > * doc/tm.texi: Regenerate.
> > * omp-simd-clone.c (simd_clone_mangle): Call vec_base_name hook.
> > * target.def (vec_base_name): New hook.
> > * targhooks.c (cgraph.h): New include.
> > (default_vec_base_name): New function.
> > * targhooks.h (default_vec_base_name): New function declaration.
> >

Jakub

Re: RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME

[Richard Biener]

On Mon, Mar 11, 2019 at 11:39 PM Steve Ellcey  wrote:
>
> This is a proposed GCC patch that allows targets to modify the names of
> the libmvec routines that get called.  Currently, if you build ToT GCC
> on Aarch64 and include this glibc patch:
>
> https://sourceware.org/ml/libc-alpha/2019-03/msg00106.html
>
> And then compile a call to expf which gets vectorized, GCC will
> generate a libmvec call to '_ZGVnN4v___expf_finite' instead of
> _ZGVnN4v_expf because the limvec name is based on the assembly name
> of the scalar function (__expf_finite) and not the 'real' name (expf).
> This means that libmvec needs to provide both names, even though the
> routines don't differ.
>
> Rather than create both names I would like to make it possible for
> GCC to generate calls to libmvec based on the real name by having
> a target specific function that allows GCC to use the DECL_NAME instead
> of DECL_ASSEMBLER_NAME to create the libmvec name.
>
> The responses to my glibc patch (referenced above) has a pointer
> to where this was discussed in the GCC mailing list a couple of years
> ago:
>
> https://gcc.gnu.org/ml/gcc/2015-06/msg00173.html
>
> and which has a pointer back to an older glibc string as well:
>
> https://sourceware.org/ml/libc-alpha/2015-06/msg00213.html
>
> Any thoughts on this patch as a way of 'fixing' GCC to not use the
> finite alias names?

I think this needs to be fixed on the glibc side - if glibc advertises

float expf (float, float)
__attribute__((simd(notinbranch),alias("__expf_finite"))

or so then you of course have to provide an implementation that matches this.

It shouldn't be difficult to provide an alias from the glibc side, no?  How does
x86 avoid this issue?

Richard.

> Steve Ellcey
> sell...@marvell.com
>
>
> 2018-03-11  Steve Ellcey  
>
> * config/aarch64/aarch64.c (aarch64_simd_clone_vec_base_name):
> New function.
> (TARGET_SIMD_CLONE_VEC_BASE_NAME): New macro.
> * doc/tm.texi.in (TARGET_SIMD_CLONE_VEC_BASE_NAME): New hook.
> * doc/tm.texi: Regenerate.
> * omp-simd-clone.c (simd_clone_mangle): Call vec_base_name hook.
> * target.def (vec_base_name): New hook.
> * targhooks.c (cgraph.h): New include.
> (default_vec_base_name): New function.
> * targhooks.h (default_vec_base_name): New function declaration.
>