diff mbox series

[1/3] powerpc64: Replace some PPC_FEATURE_HAS_VSX with PPC_FEATURE_ARCH_2_06

Message ID 20210706105107.1866836-1-anton@ozlabs.org
State Changes Requested
Delegated to: Tulio Magno Quites Machado Filho
Headers show
Series [1/3] powerpc64: Replace some PPC_FEATURE_HAS_VSX with PPC_FEATURE_ARCH_2_06 | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Anton Blanchard July 6, 2021, 10:51 a.m. UTC
We use PPC_FEATURE_HAS_VSX to select a number of POWER7 optimised
functions. These functions don't use any VSX instructions, so
PPC_FEATURE_ARCH_2_06 seems like a better fit.
---
 sysdeps/powerpc/powerpc64/multiarch/memchr.c       | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/memcmp.c       | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/memrchr.c      | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/memset.c       | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/rawmemchr.c    | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/stpncpy.c      | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strcasecmp.c   | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strcasecmp_l.c | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strchr.c       | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strchrnul.c    | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strcmp.c       | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strlen.c       | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strncase.c     | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strncase_l.c   | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strncmp.c      | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strncpy.c      | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strnlen.c      | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strrchr.c      | 2 +-
 sysdeps/powerpc/powerpc64/multiarch/strstr.c       | 2 +-
 19 files changed, 19 insertions(+), 19 deletions(-)

Comments

Adhemerval Zanella July 6, 2021, 3:05 p.m. UTC | #1
On 06/07/2021 07:51, Anton Blanchard via Libc-alpha wrote:
> We use PPC_FEATURE_HAS_VSX to select a number of POWER7 optimised
> functions. These functions don't use any VSX instructions, so
> PPC_FEATURE_ARCH_2_06 seems like a better fit.

I think we should rename the symbols to something more meanifull as
well, like 'symbol_vsx'.  I take the idea is to support powerpc 
variants with ISA 2.06 that do not support VSX.

The patch looks good thanks and I assume you are testing on microwatt
(so you would see a failure is VSX is actually being used).

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  sysdeps/powerpc/powerpc64/multiarch/memchr.c       | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/memcmp.c       | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/memrchr.c      | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/memset.c       | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/rawmemchr.c    | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/stpncpy.c      | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strcasecmp.c   | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strcasecmp_l.c | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strchr.c       | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strchrnul.c    | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strcmp.c       | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strlen.c       | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strncase.c     | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strncase_l.c   | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strncmp.c      | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strncpy.c      | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strnlen.c      | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strrchr.c      | 2 +-
>  sysdeps/powerpc/powerpc64/multiarch/strstr.c       | 2 +-
>  19 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/memchr.c b/sysdeps/powerpc/powerpc64/multiarch/memchr.c
> index 0c718d4f15..c24186689e 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/memchr.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/memchr.c
> @@ -30,7 +30,7 @@ extern __typeof (__memchr) __memchr_power8 attribute_hidden;
>  libc_ifunc (__memchr,
>  	    (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  	    ? __memchr_power8 :
> -	    (hwcap & PPC_FEATURE_HAS_VSX)
> +	    (hwcap & PPC_FEATURE_ARCH_2_06)
>              ? __memchr_power7
>              : __memchr_ppc);
>  
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/memcmp.c b/sysdeps/powerpc/powerpc64/multiarch/memcmp.c
> index 4fd089aba7..99559bce26 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/memcmp.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/memcmp.c
> @@ -40,7 +40,7 @@ libc_ifunc_redirected (__redirect_memcmp, memcmp,
>  #endif
>  		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  		       ? __memcmp_power8 :
> -		       (hwcap & PPC_FEATURE_HAS_VSX)
> +		       (hwcap & PPC_FEATURE_ARCH_2_06)
>  		       ? __memcmp_power7
>  		       : (hwcap & PPC_FEATURE_POWER4)
>  			 ? __memcmp_power4
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/memrchr.c b/sysdeps/powerpc/powerpc64/multiarch/memrchr.c
> index e06d6468b8..16bb6f0042 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/memrchr.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/memrchr.c
> @@ -30,7 +30,7 @@ extern __typeof (__memrchr) __memrchr_power8 attribute_hidden;
>  libc_ifunc (__memrchr,
>  	    (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  	    ? __memrchr_power8 :
> -	      (hwcap & PPC_FEATURE_HAS_VSX)
> +	      (hwcap & PPC_FEATURE_ARCH_2_06)
>  	      ? __memrchr_power7
>  	    : __memrchr_ppc);
>  
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/memset.c b/sysdeps/powerpc/powerpc64/multiarch/memset.c
> index 5994bf02e6..c1aa143f60 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/memset.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/memset.c
> @@ -48,7 +48,7 @@ libc_ifunc (__libc_memset,
>  # endif
>              (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>              ? __memset_power8 :
> -	      (hwcap & PPC_FEATURE_HAS_VSX)
> +	      (hwcap & PPC_FEATURE_ARCH_2_06)
>  	      ? __memset_power7 :
>  		(hwcap & PPC_FEATURE_ARCH_2_05)
>  		? __memset_power6 :
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/rawmemchr.c b/sysdeps/powerpc/powerpc64/multiarch/rawmemchr.c
> index c0ffea2b93..b5d2d3a635 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/rawmemchr.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/rawmemchr.c
> @@ -41,7 +41,7 @@ libc_ifunc_redirected (__redirect___rawmemchr, __rawmemchr,
>  		       (hwcap2 & PPC_FEATURE2_ARCH_3_00)
>  		       ? __rawmemchr_power9 :
>  # endif
> -		         (hwcap & PPC_FEATURE_HAS_VSX)
> +		         (hwcap & PPC_FEATURE_ARCH_2_06)
>  		         ? __rawmemchr_power7
>  		       : __rawmemchr_ppc);
>  
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c b/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c
> index bebd377fd9..e7035761a7 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c
> @@ -40,7 +40,7 @@ libc_ifunc_redirected (__redirect___stpncpy, __stpncpy,
>  # endif
>  		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  		       ? __stpncpy_power8
> -		       : (hwcap & PPC_FEATURE_HAS_VSX)
> +		       : (hwcap & PPC_FEATURE_ARCH_2_06)
>  			 ? __stpncpy_power7
>  			 : __stpncpy_ppc);
>  weak_alias (__stpncpy, stpncpy)
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strcasecmp.c b/sysdeps/powerpc/powerpc64/multiarch/strcasecmp.c
> index dcd7774403..55ca6c85c4 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strcasecmp.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strcasecmp.c
> @@ -29,7 +29,7 @@ extern __typeof (__strcasecmp) __strcasecmp_power8 attribute_hidden;
>  libc_ifunc (__libc_strcasecmp,
>  	     (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>               ? __strcasecmp_power8:
> -	     (hwcap & PPC_FEATURE_HAS_VSX)
> +	     (hwcap & PPC_FEATURE_ARCH_2_06)
>               ? __strcasecmp_power7
>               : __strcasecmp_ppc);
>  
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strcasecmp_l.c b/sysdeps/powerpc/powerpc64/multiarch/strcasecmp_l.c
> index 96a70b8b11..1afee5d7fd 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strcasecmp_l.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strcasecmp_l.c
> @@ -32,7 +32,7 @@ extern __typeof (__strcasecmp_l) __strcasecmp_l_power7 attribute_hidden;
>  
>  extern __typeof (__strcasecmp_l) __libc_strcasecmp_l;
>  libc_ifunc (__libc_strcasecmp_l,
> -	    (hwcap & PPC_FEATURE_HAS_VSX)
> +	    (hwcap & PPC_FEATURE_ARCH_2_06)
>              ? __strcasecmp_l_power7
>              : __strcasecmp_l_ppc);
>  
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strchr.c b/sysdeps/powerpc/powerpc64/multiarch/strchr.c
> index ea9ac1134f..27c794c6b7 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strchr.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strchr.c
> @@ -35,7 +35,7 @@ extern __typeof (strchr) __strchr_power8 attribute_hidden;
>  libc_ifunc_redirected (__redirect_strchr, strchr,
>  		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  		       ? __strchr_power8 :
> -		       (hwcap & PPC_FEATURE_HAS_VSX)
> +		       (hwcap & PPC_FEATURE_ARCH_2_06)
>  		       ? __strchr_power7
>  		       : __strchr_ppc);
>  weak_alias (strchr, index)
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strchrnul.c b/sysdeps/powerpc/powerpc64/multiarch/strchrnul.c
> index 4688e7c3f0..4a07b4a242 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strchrnul.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strchrnul.c
> @@ -30,7 +30,7 @@ extern __typeof (__strchrnul) __strchrnul_power8 attribute_hidden;
>  libc_ifunc (__strchrnul,
>  	    (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  	    ? __strchrnul_power8 :
> -	    (hwcap & PPC_FEATURE_HAS_VSX)
> +	    (hwcap & PPC_FEATURE_ARCH_2_06)
>              ? __strchrnul_power7
>              : __strchrnul_ppc);
>  
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strcmp.c b/sysdeps/powerpc/powerpc64/multiarch/strcmp.c
> index 72f9a639bf..4b0b25fff6 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strcmp.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strcmp.c
> @@ -40,7 +40,7 @@ libc_ifunc_redirected (__redirect_strcmp, strcmp,
>  # endif
>  		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  		       ? __strcmp_power8
> -		       : (hwcap & PPC_FEATURE_HAS_VSX)
> +		       : (hwcap & PPC_FEATURE_ARCH_2_06)
>  			 ? __strcmp_power7
>  			 : __strcmp_ppc);
>  #endif
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strlen.c b/sysdeps/powerpc/powerpc64/multiarch/strlen.c
> index 109c8a90bd..0cd1c6faff 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strlen.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strlen.c
> @@ -42,7 +42,7 @@ libc_ifunc (__libc_strlen,
>  # endif
>  	    (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  	    ? __strlen_power8 :
> -	      (hwcap & PPC_FEATURE_HAS_VSX)
> +	      (hwcap & PPC_FEATURE_ARCH_2_06)
>  	      ? __strlen_power7
>  	      : __strlen_ppc);
>  
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strncase.c b/sysdeps/powerpc/powerpc64/multiarch/strncase.c
> index 2013a5d75a..644046bd74 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strncase.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strncase.c
> @@ -29,7 +29,7 @@ extern __typeof (__strncasecmp) __strncasecmp_power8 attribute_hidden;
>  libc_ifunc (__libc_strncasecmp,
>  	     (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>               ? __strncasecmp_power8:
> -	     (hwcap & PPC_FEATURE_HAS_VSX)
> +	     (hwcap & PPC_FEATURE_ARCH_2_06)
>               ? __strncasecmp_power7
>               : __strncasecmp_ppc);
>  
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strncase_l.c b/sysdeps/powerpc/powerpc64/multiarch/strncase_l.c
> index cad6da302d..d2d761af72 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strncase_l.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strncase_l.c
> @@ -34,7 +34,7 @@ extern __typeof (__strncasecmp_l) __strncasecmp_l_power7 attribute_hidden;
>     ifunc symbol properly.  */
>  extern __typeof (__strncasecmp_l) __libc_strncasecmp_l;
>  libc_ifunc (__libc_strncasecmp_l,
> -	     (hwcap & PPC_FEATURE_HAS_VSX)
> +	     (hwcap & PPC_FEATURE_ARCH_2_06)
>               ? __strncasecmp_l_power7
>               : __strncasecmp_l_ppc);
>  
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strncmp.c b/sysdeps/powerpc/powerpc64/multiarch/strncmp.c
> index eef524ddfb..1f689e5c05 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strncmp.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strncmp.c
> @@ -43,7 +43,7 @@ libc_ifunc_redirected (__redirect_strncmp, strncmp,
>  # endif
>  		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  		       ? __strncmp_power8
> -		       : (hwcap & PPC_FEATURE_HAS_VSX)
> +		       : (hwcap & PPC_FEATURE_ARCH_2_06)
>  			 ? __strncmp_power7
>  			 : (hwcap & PPC_FEATURE_POWER4)
>  			   ? __strncmp_power4
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strncpy.c b/sysdeps/powerpc/powerpc64/multiarch/strncpy.c
> index 7da9def358..d4d3463bd1 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strncpy.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strncpy.c
> @@ -43,7 +43,7 @@ libc_ifunc_redirected (__redirect_strncpy, strncpy,
>  # endif
>  		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  		       ? __strncpy_power8
> -		       : (hwcap & PPC_FEATURE_HAS_VSX)
> +		       : (hwcap & PPC_FEATURE_ARCH_2_06)
>  			 ? __strncpy_power7
>  			 : __strncpy_ppc);
>  
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strnlen.c b/sysdeps/powerpc/powerpc64/multiarch/strnlen.c
> index 264b7a752d..baf375a75a 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strnlen.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strnlen.c
> @@ -31,7 +31,7 @@ extern __typeof (__strnlen) __strnlen_power8 attribute_hidden;
>  libc_ifunc_redirected (__redirect___strnlen, __strnlen,
>  		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  		       ? __strnlen_power8 :
> -			 (hwcap & PPC_FEATURE_HAS_VSX)
> +			 (hwcap & PPC_FEATURE_ARCH_2_06)
>  			 ? __strnlen_power7
>  			 : __strnlen_ppc);
>  weak_alias (__strnlen, strnlen)
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strrchr.c b/sysdeps/powerpc/powerpc64/multiarch/strrchr.c
> index bb06b93d19..1c9eea1817 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strrchr.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strrchr.c
> @@ -33,7 +33,7 @@ extern __typeof (strrchr) __strrchr_power8 attribute_hidden;
>  libc_ifunc_redirected (__redirect_strrchr, strrchr,
>  		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
>  		       ? __strrchr_power8 :
> -		       (hwcap & PPC_FEATURE_HAS_VSX)
> +		       (hwcap & PPC_FEATURE_ARCH_2_06)
>  		       ? __strrchr_power7
>  		       : __strrchr_ppc);
>  weak_alias (strrchr, rindex)
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/strstr.c b/sysdeps/powerpc/powerpc64/multiarch/strstr.c
> index bb0588844e..6582798dda 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/strstr.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/strstr.c
> @@ -30,7 +30,7 @@ extern __typeof (strstr) __strstr_power7 attribute_hidden;
>  /* Avoid DWARF definition DIE on ifunc symbol so that GDB can handle
>     ifunc symbol properly.  */
>  libc_ifunc_redirected (__redirect_strstr, strstr,
> -		       (hwcap & PPC_FEATURE_HAS_VSX)
> +		       (hwcap & PPC_FEATURE_ARCH_2_06)
>  		       ? __strstr_power7
>  		       : __strstr_ppc);
>  #endif
>
Tulio Magno Quites Machado Filho July 8, 2021, 10:29 p.m. UTC | #2
Anton Blanchard via Libc-alpha <libc-alpha@sourceware.org> writes:

> We use PPC_FEATURE_HAS_VSX to select a number of POWER7 optimised
> functions. These functions don't use any VSX instructions, so
> PPC_FEATURE_ARCH_2_06 seems like a better fit.

I don't think we can replace PPC_FEATURE_HAS_VSX with PPC_FEATURE_ARCH_2_06
because it would cause issues similar to the ones fixed in patch 3/3, i.e.
this function would run on a processor that implements the Power ISA 2.06
but does not implement Altivec.

So, if testing for PPC_FEATURE_ARCH_2_06 is important, I think it has to test
for:

    (hwcap & PPC_FEATURE_ARCH_2_06 && hwcap & PPC_FEATURE_HAS_ALTIVEC)

However, VSX was introduced by Power ISA 2.06. So, PPC_FEATURE_HAS_VSX
implies PPC_FEATURE_ARCH_2_06.  PPC_FEATURE_HAS_VSX also implies
PPC_FEATURE_HAS_ALTIVEC.
The change would only make a difference if a processor implements both Power
ISA 2.06 and Altivec, but does not implement VSX.

Am I missing anything?

Another important point is to keep the tests in the IFUNC resolvers in sync
with sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c
Otherwise, glibc tests and benchtests may not execute correctly.
Adhemerval Zanella July 13, 2021, 7:03 p.m. UTC | #3
On 08/07/2021 19:29, Tulio Magno Quites Machado Filho via Libc-alpha wrote:
> Anton Blanchard via Libc-alpha <libc-alpha@sourceware.org> writes:
> 
>> We use PPC_FEATURE_HAS_VSX to select a number of POWER7 optimised
>> functions. These functions don't use any VSX instructions, so
>> PPC_FEATURE_ARCH_2_06 seems like a better fit.
> 
> I don't think we can replace PPC_FEATURE_HAS_VSX with PPC_FEATURE_ARCH_2_06
> because it would cause issues similar to the ones fixed in patch 3/3, i.e.
> this function would run on a processor that implements the Power ISA 2.06
> but does not implement Altivec.
> 
> So, if testing for PPC_FEATURE_ARCH_2_06 is important, I think it has to test
> for:
> 
>     (hwcap & PPC_FEATURE_ARCH_2_06 && hwcap & PPC_FEATURE_HAS_ALTIVEC)
> 
> However, VSX was introduced by Power ISA 2.06. So, PPC_FEATURE_HAS_VSX
> implies PPC_FEATURE_ARCH_2_06.  PPC_FEATURE_HAS_VSX also implies
> PPC_FEATURE_HAS_ALTIVEC.
> The change would only make a difference if a processor implements both Power
> ISA 2.06 and Altivec, but does not implement VSX.
> 
> Am I missing anything?

My understanding is the selection change done for this patch is to enable 
routines that do *not* use vectorized instructions (either VSX or altivec)
on a CPU that does have isa 2.06 (to use cmpb for instance).  That's why
I suggested that we should in fact rename them and also maybe move to
a different folder than 'power7' to something more meaningful (for instance
__memchr_isa206 or something).

The selection change above you suggested is not strictly required, it
would be for memcpy/memmove (if I recall correctly they only use altivec).

> 
> Another important point is to keep the tests in the IFUNC resolvers in sync
> with sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c
> Otherwise, glibc tests and benchtests may not execute correctly.
>
Tulio Magno Quites Machado Filho July 16, 2021, 1:28 p.m. UTC | #4
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:

> On 08/07/2021 19:29, Tulio Magno Quites Machado Filho via Libc-alpha wrote:
>> I don't think we can replace PPC_FEATURE_HAS_VSX with PPC_FEATURE_ARCH_2_06
>> because it would cause issues similar to the ones fixed in patch 3/3, i.e.
>> this function would run on a processor that implements the Power ISA 2.06
>> but does not implement Altivec.
>> 
>> So, if testing for PPC_FEATURE_ARCH_2_06 is important, I think it has to test
>> for:
>> 
>>     (hwcap & PPC_FEATURE_ARCH_2_06 && hwcap & PPC_FEATURE_HAS_ALTIVEC)
>> 
>> However, VSX was introduced by Power ISA 2.06. So, PPC_FEATURE_HAS_VSX
>> implies PPC_FEATURE_ARCH_2_06.  PPC_FEATURE_HAS_VSX also implies
>> PPC_FEATURE_HAS_ALTIVEC.
>> The change would only make a difference if a processor implements both Power
>> ISA 2.06 and Altivec, but does not implement VSX.
>> 
>> Am I missing anything?
>
> My understanding is the selection change done for this patch is to enable 
> routines that do *not* use vectorized instructions (either VSX or altivec)
> on a CPU that does have isa 2.06 (to use cmpb for instance).

Yes. My previous comment would only make sense for functions using Altivec,
but these functions are not.  I'm sorry.

> That's why
> I suggested that we should in fact rename them and also maybe move to
> a different folder than 'power7' to something more meaningful (for instance
> __memchr_isa206 or something).

I don't have a strong opinion on this.  LGTM either way.

>> Another important point is to keep the tests in the IFUNC resolvers in sync
>> with sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c
>> Otherwise, glibc tests and benchtests may not execute correctly.

Heads up this paragraph is still valid.
Anton Blanchard July 26, 2021, 11:23 p.m. UTC | #5
Hi,

> >> Another important point is to keep the tests in the IFUNC
> >> resolvers in sync with
> >> sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c Otherwise,
> >> glibc tests and benchtests may not execute correctly.  
> 
> Heads up this paragraph is still valid.

Oops, thanks Adhemerval and Tulio for pointing this out. Will fix and
resubmit.

Thanks,
Anton
diff mbox series

Patch

diff --git a/sysdeps/powerpc/powerpc64/multiarch/memchr.c b/sysdeps/powerpc/powerpc64/multiarch/memchr.c
index 0c718d4f15..c24186689e 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/memchr.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/memchr.c
@@ -30,7 +30,7 @@  extern __typeof (__memchr) __memchr_power8 attribute_hidden;
 libc_ifunc (__memchr,
 	    (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 	    ? __memchr_power8 :
-	    (hwcap & PPC_FEATURE_HAS_VSX)
+	    (hwcap & PPC_FEATURE_ARCH_2_06)
             ? __memchr_power7
             : __memchr_ppc);
 
diff --git a/sysdeps/powerpc/powerpc64/multiarch/memcmp.c b/sysdeps/powerpc/powerpc64/multiarch/memcmp.c
index 4fd089aba7..99559bce26 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/memcmp.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/memcmp.c
@@ -40,7 +40,7 @@  libc_ifunc_redirected (__redirect_memcmp, memcmp,
 #endif
 		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 		       ? __memcmp_power8 :
-		       (hwcap & PPC_FEATURE_HAS_VSX)
+		       (hwcap & PPC_FEATURE_ARCH_2_06)
 		       ? __memcmp_power7
 		       : (hwcap & PPC_FEATURE_POWER4)
 			 ? __memcmp_power4
diff --git a/sysdeps/powerpc/powerpc64/multiarch/memrchr.c b/sysdeps/powerpc/powerpc64/multiarch/memrchr.c
index e06d6468b8..16bb6f0042 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/memrchr.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/memrchr.c
@@ -30,7 +30,7 @@  extern __typeof (__memrchr) __memrchr_power8 attribute_hidden;
 libc_ifunc (__memrchr,
 	    (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 	    ? __memrchr_power8 :
-	      (hwcap & PPC_FEATURE_HAS_VSX)
+	      (hwcap & PPC_FEATURE_ARCH_2_06)
 	      ? __memrchr_power7
 	    : __memrchr_ppc);
 
diff --git a/sysdeps/powerpc/powerpc64/multiarch/memset.c b/sysdeps/powerpc/powerpc64/multiarch/memset.c
index 5994bf02e6..c1aa143f60 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/memset.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/memset.c
@@ -48,7 +48,7 @@  libc_ifunc (__libc_memset,
 # endif
             (hwcap2 & PPC_FEATURE2_ARCH_2_07)
             ? __memset_power8 :
-	      (hwcap & PPC_FEATURE_HAS_VSX)
+	      (hwcap & PPC_FEATURE_ARCH_2_06)
 	      ? __memset_power7 :
 		(hwcap & PPC_FEATURE_ARCH_2_05)
 		? __memset_power6 :
diff --git a/sysdeps/powerpc/powerpc64/multiarch/rawmemchr.c b/sysdeps/powerpc/powerpc64/multiarch/rawmemchr.c
index c0ffea2b93..b5d2d3a635 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/rawmemchr.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/rawmemchr.c
@@ -41,7 +41,7 @@  libc_ifunc_redirected (__redirect___rawmemchr, __rawmemchr,
 		       (hwcap2 & PPC_FEATURE2_ARCH_3_00)
 		       ? __rawmemchr_power9 :
 # endif
-		         (hwcap & PPC_FEATURE_HAS_VSX)
+		         (hwcap & PPC_FEATURE_ARCH_2_06)
 		         ? __rawmemchr_power7
 		       : __rawmemchr_ppc);
 
diff --git a/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c b/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c
index bebd377fd9..e7035761a7 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c
@@ -40,7 +40,7 @@  libc_ifunc_redirected (__redirect___stpncpy, __stpncpy,
 # endif
 		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 		       ? __stpncpy_power8
-		       : (hwcap & PPC_FEATURE_HAS_VSX)
+		       : (hwcap & PPC_FEATURE_ARCH_2_06)
 			 ? __stpncpy_power7
 			 : __stpncpy_ppc);
 weak_alias (__stpncpy, stpncpy)
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strcasecmp.c b/sysdeps/powerpc/powerpc64/multiarch/strcasecmp.c
index dcd7774403..55ca6c85c4 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strcasecmp.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strcasecmp.c
@@ -29,7 +29,7 @@  extern __typeof (__strcasecmp) __strcasecmp_power8 attribute_hidden;
 libc_ifunc (__libc_strcasecmp,
 	     (hwcap2 & PPC_FEATURE2_ARCH_2_07)
              ? __strcasecmp_power8:
-	     (hwcap & PPC_FEATURE_HAS_VSX)
+	     (hwcap & PPC_FEATURE_ARCH_2_06)
              ? __strcasecmp_power7
              : __strcasecmp_ppc);
 
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strcasecmp_l.c b/sysdeps/powerpc/powerpc64/multiarch/strcasecmp_l.c
index 96a70b8b11..1afee5d7fd 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strcasecmp_l.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strcasecmp_l.c
@@ -32,7 +32,7 @@  extern __typeof (__strcasecmp_l) __strcasecmp_l_power7 attribute_hidden;
 
 extern __typeof (__strcasecmp_l) __libc_strcasecmp_l;
 libc_ifunc (__libc_strcasecmp_l,
-	    (hwcap & PPC_FEATURE_HAS_VSX)
+	    (hwcap & PPC_FEATURE_ARCH_2_06)
             ? __strcasecmp_l_power7
             : __strcasecmp_l_ppc);
 
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strchr.c b/sysdeps/powerpc/powerpc64/multiarch/strchr.c
index ea9ac1134f..27c794c6b7 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strchr.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strchr.c
@@ -35,7 +35,7 @@  extern __typeof (strchr) __strchr_power8 attribute_hidden;
 libc_ifunc_redirected (__redirect_strchr, strchr,
 		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 		       ? __strchr_power8 :
-		       (hwcap & PPC_FEATURE_HAS_VSX)
+		       (hwcap & PPC_FEATURE_ARCH_2_06)
 		       ? __strchr_power7
 		       : __strchr_ppc);
 weak_alias (strchr, index)
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strchrnul.c b/sysdeps/powerpc/powerpc64/multiarch/strchrnul.c
index 4688e7c3f0..4a07b4a242 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strchrnul.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strchrnul.c
@@ -30,7 +30,7 @@  extern __typeof (__strchrnul) __strchrnul_power8 attribute_hidden;
 libc_ifunc (__strchrnul,
 	    (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 	    ? __strchrnul_power8 :
-	    (hwcap & PPC_FEATURE_HAS_VSX)
+	    (hwcap & PPC_FEATURE_ARCH_2_06)
             ? __strchrnul_power7
             : __strchrnul_ppc);
 
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strcmp.c b/sysdeps/powerpc/powerpc64/multiarch/strcmp.c
index 72f9a639bf..4b0b25fff6 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strcmp.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strcmp.c
@@ -40,7 +40,7 @@  libc_ifunc_redirected (__redirect_strcmp, strcmp,
 # endif
 		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 		       ? __strcmp_power8
-		       : (hwcap & PPC_FEATURE_HAS_VSX)
+		       : (hwcap & PPC_FEATURE_ARCH_2_06)
 			 ? __strcmp_power7
 			 : __strcmp_ppc);
 #endif
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strlen.c b/sysdeps/powerpc/powerpc64/multiarch/strlen.c
index 109c8a90bd..0cd1c6faff 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strlen.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strlen.c
@@ -42,7 +42,7 @@  libc_ifunc (__libc_strlen,
 # endif
 	    (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 	    ? __strlen_power8 :
-	      (hwcap & PPC_FEATURE_HAS_VSX)
+	      (hwcap & PPC_FEATURE_ARCH_2_06)
 	      ? __strlen_power7
 	      : __strlen_ppc);
 
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strncase.c b/sysdeps/powerpc/powerpc64/multiarch/strncase.c
index 2013a5d75a..644046bd74 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strncase.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strncase.c
@@ -29,7 +29,7 @@  extern __typeof (__strncasecmp) __strncasecmp_power8 attribute_hidden;
 libc_ifunc (__libc_strncasecmp,
 	     (hwcap2 & PPC_FEATURE2_ARCH_2_07)
              ? __strncasecmp_power8:
-	     (hwcap & PPC_FEATURE_HAS_VSX)
+	     (hwcap & PPC_FEATURE_ARCH_2_06)
              ? __strncasecmp_power7
              : __strncasecmp_ppc);
 
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strncase_l.c b/sysdeps/powerpc/powerpc64/multiarch/strncase_l.c
index cad6da302d..d2d761af72 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strncase_l.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strncase_l.c
@@ -34,7 +34,7 @@  extern __typeof (__strncasecmp_l) __strncasecmp_l_power7 attribute_hidden;
    ifunc symbol properly.  */
 extern __typeof (__strncasecmp_l) __libc_strncasecmp_l;
 libc_ifunc (__libc_strncasecmp_l,
-	     (hwcap & PPC_FEATURE_HAS_VSX)
+	     (hwcap & PPC_FEATURE_ARCH_2_06)
              ? __strncasecmp_l_power7
              : __strncasecmp_l_ppc);
 
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strncmp.c b/sysdeps/powerpc/powerpc64/multiarch/strncmp.c
index eef524ddfb..1f689e5c05 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strncmp.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strncmp.c
@@ -43,7 +43,7 @@  libc_ifunc_redirected (__redirect_strncmp, strncmp,
 # endif
 		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 		       ? __strncmp_power8
-		       : (hwcap & PPC_FEATURE_HAS_VSX)
+		       : (hwcap & PPC_FEATURE_ARCH_2_06)
 			 ? __strncmp_power7
 			 : (hwcap & PPC_FEATURE_POWER4)
 			   ? __strncmp_power4
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strncpy.c b/sysdeps/powerpc/powerpc64/multiarch/strncpy.c
index 7da9def358..d4d3463bd1 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strncpy.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strncpy.c
@@ -43,7 +43,7 @@  libc_ifunc_redirected (__redirect_strncpy, strncpy,
 # endif
 		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 		       ? __strncpy_power8
-		       : (hwcap & PPC_FEATURE_HAS_VSX)
+		       : (hwcap & PPC_FEATURE_ARCH_2_06)
 			 ? __strncpy_power7
 			 : __strncpy_ppc);
 
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strnlen.c b/sysdeps/powerpc/powerpc64/multiarch/strnlen.c
index 264b7a752d..baf375a75a 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strnlen.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strnlen.c
@@ -31,7 +31,7 @@  extern __typeof (__strnlen) __strnlen_power8 attribute_hidden;
 libc_ifunc_redirected (__redirect___strnlen, __strnlen,
 		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 		       ? __strnlen_power8 :
-			 (hwcap & PPC_FEATURE_HAS_VSX)
+			 (hwcap & PPC_FEATURE_ARCH_2_06)
 			 ? __strnlen_power7
 			 : __strnlen_ppc);
 weak_alias (__strnlen, strnlen)
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strrchr.c b/sysdeps/powerpc/powerpc64/multiarch/strrchr.c
index bb06b93d19..1c9eea1817 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strrchr.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strrchr.c
@@ -33,7 +33,7 @@  extern __typeof (strrchr) __strrchr_power8 attribute_hidden;
 libc_ifunc_redirected (__redirect_strrchr, strrchr,
 		       (hwcap2 & PPC_FEATURE2_ARCH_2_07)
 		       ? __strrchr_power8 :
-		       (hwcap & PPC_FEATURE_HAS_VSX)
+		       (hwcap & PPC_FEATURE_ARCH_2_06)
 		       ? __strrchr_power7
 		       : __strrchr_ppc);
 weak_alias (strrchr, rindex)
diff --git a/sysdeps/powerpc/powerpc64/multiarch/strstr.c b/sysdeps/powerpc/powerpc64/multiarch/strstr.c
index bb0588844e..6582798dda 100644
--- a/sysdeps/powerpc/powerpc64/multiarch/strstr.c
+++ b/sysdeps/powerpc/powerpc64/multiarch/strstr.c
@@ -30,7 +30,7 @@  extern __typeof (strstr) __strstr_power7 attribute_hidden;
 /* Avoid DWARF definition DIE on ifunc symbol so that GDB can handle
    ifunc symbol properly.  */
 libc_ifunc_redirected (__redirect_strstr, strstr,
-		       (hwcap & PPC_FEATURE_HAS_VSX)
+		       (hwcap & PPC_FEATURE_ARCH_2_06)
 		       ? __strstr_power7
 		       : __strstr_ppc);
 #endif