| Message ID | 41abe64e531d6d590fd85d9917b62bf2f169d1d1.camel@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: Patch to allow use of DECL_NAME in libmvec calls instead of DECL_ASSEMBER_NAME | expand |
On Mon, Mar 11, 2019 at 11:39 PM Steve Ellcey <sellcey@marvell.com> 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 > sellcey@marvell.com > > > 2018-03-11 Steve Ellcey <sellcey@marvell.com> > > * 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. >
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 <sellcey@marvell.com> > > > > * 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
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 <math.h> 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
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 <https://gcc.gnu.org/ml/gcc/2015-06/msg00176.html> - 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.
On Tue, 2019-03-12 at 14:17 +0000, 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 > <https://gcc.gnu.org/ml/gcc/2015-06/msg00176.html> - 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 (&pp); - 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 (&pp, 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
On Tue, Mar 12, 2019 at 11:21:26PM +0000, 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
On Wed, Mar 13, 2019 at 12:40 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Tue, Mar 12, 2019 at 11:21:26PM +0000, 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
On Wed, Mar 13, 2019 at 01:01:54PM +0100, Richard Biener wrote: > On Wed, Mar 13, 2019 at 12:40 AM Jakub Jelinek <jakub@redhat.com> wrote: > > > > On Tue, Mar 12, 2019 at 11:21:26PM +0000, 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
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).
On Wed, Mar 13, 2019 at 01:00:58PM +0000, 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
On Wed, Mar 13, 2019 at 02:07:29PM +0100, Jakub Jelinek wrote: > On Wed, Mar 13, 2019 at 01:00:58PM +0000, 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
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.
On Wed, 2019-03-13 at 14:38 +0000, 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 sellcey@marvell.com
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.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 252bed7..cddab80 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -18711,6 +18711,14 @@ aarch64_simd_clone_usable (struct cgraph_node *node) } } +/* Implement TARGET_SIMD_CLONE_VEC_BASE_NAME */ + +static const char * +aarch64_simd_clone_vec_base_name (struct cgraph_node *node) +{ + return IDENTIFIER_POINTER (DECL_NAME (node->decl)); +} + /* Implement TARGET_COMP_TYPE_ATTRIBUTES */ static int @@ -19251,6 +19259,9 @@ aarch64_libgcc_floating_mode_supported_p #undef TARGET_SIMD_CLONE_USABLE #define TARGET_SIMD_CLONE_USABLE aarch64_simd_clone_usable +#undef TARGET_SIMD_CLONE_VEC_BASE_NAME +#define TARGET_SIMD_CLONE_VEC_BASE_NAME aarch64_simd_clone_vec_base_name + #undef TARGET_COMP_TYPE_ATTRIBUTES #define TARGET_COMP_TYPE_ATTRIBUTES aarch64_comp_type_attributes diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in index fe1194e..de4bdb42 100644 --- a/gcc/doc/tm.texi.in +++ b/gcc/doc/tm.texi.in @@ -4196,6 +4196,8 @@ address; but often a machine-dependent strategy can generate better code. @hook TARGET_SIMD_CLONE_USABLE +@hook TARGET_SIMD_CLONE_VEC_BASE_NAME + @hook TARGET_SIMT_VF @hook TARGET_GOACC_VALIDATE_DIMS diff --git a/gcc/omp-simd-clone.c b/gcc/omp-simd-clone.c index 388198b..b3a57aa 100644 --- a/gcc/omp-simd-clone.c +++ b/gcc/omp-simd-clone.c @@ -409,7 +409,7 @@ simd_clone_mangle (struct cgraph_node *node, } pp_underscore (&pp); - const char *str = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (node->decl)); + const char *str = targetm.simd_clone.vec_base_name (node); if (*str == '*') ++str; pp_string (&pp, str); diff --git a/gcc/target.def b/gcc/target.def index 66cee07..da60249 100644 --- a/gcc/target.def +++ b/gcc/target.def @@ -1655,6 +1655,13 @@ usable. In that case, the smaller the number is, the more desirable it is\n\ to use it.", int, (struct cgraph_node *), NULL) +DEFHOOK +(vec_base_name, +"This hook should return the name of the scalar function being cloned.\n\ +This defaults to DECL_ASSEMBLER_NAME, but targets could use DECL_NAME\n\ +instead or some other variation of the function name.", +const char *, (struct cgraph_node *), default_vec_base_name) + HOOK_VECTOR_END (simd_clone) /* Functions relating to OpenMP SIMT vectorization transform. */ diff --git a/gcc/targhooks.c b/gcc/targhooks.c index 318f7e9..6792ee5 100644 --- a/gcc/targhooks.c +++ b/gcc/targhooks.c @@ -83,6 +83,7 @@ along with GCC; see the file COPYING3. If not see #include "real.h" #include "langhooks.h" #include "sbitmap.h" +#include "cgraph.h" bool default_legitimate_address_p (machine_mode mode ATTRIBUTE_UNUSED, @@ -2379,4 +2380,10 @@ default_remove_extra_call_preserved_regs (rtx_insn *, HARD_REG_SET *) { } +const char * +default_vec_base_name (struct cgraph_node * node) +{ + return IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (node->decl)); +} + #include "gt-targhooks.h" diff --git a/gcc/targhooks.h b/gcc/targhooks.h index 5943627..85dce1a 100644 --- a/gcc/targhooks.h +++ b/gcc/targhooks.h @@ -286,5 +286,6 @@ extern bool speculation_safe_value_not_needed (bool); extern rtx default_speculation_safe_value (machine_mode, rtx, rtx, rtx); extern void default_remove_extra_call_preserved_regs (rtx_insn *, HARD_REG_SET *); +extern const char * default_vec_base_name (struct cgraph_node *); #endif /* GCC_TARGHOOKS_H */
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? Steve Ellcey sellcey@marvell.com 2018-03-11 Steve Ellcey <sellcey@marvell.com> * 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.