[PATCH 2/2] powerpc: Optimzed stpncpy for POWER9
Paul E Murphy
murphyp@linux.ibm.com
Fri Aug 28 17:04:35 GMT 2020
Thank you for your contributions, I have a few minor
comments/suggestions below.
On 8/20/20 1:29 PM, Raphael Moreira Zinsly via Libc-alpha wrote:
> Adds stpncpy support into the POWER9 strncpy.
s/Adds/Add/ s/into the/to/.
Likewise, s/Optimzed/Add optimized/ in the title.
> ---
> sysdeps/powerpc/powerpc64/le/power9/stpncpy.S | 24 ++++++
> sysdeps/powerpc/powerpc64/le/power9/strncpy.S | 74 +++++++++++++++++++
> sysdeps/powerpc/powerpc64/multiarch/Makefile | 2 +-
> .../powerpc64/multiarch/ifunc-impl-list.c | 5 ++
> .../powerpc64/multiarch/stpncpy-power9.S | 24 ++++++
> sysdeps/powerpc/powerpc64/multiarch/stpncpy.c | 7 ++
> 6 files changed, 135 insertions(+), 1 deletion(-)
> create mode 100644 sysdeps/powerpc/powerpc64/le/power9/stpncpy.S
> create mode 100644 sysdeps/powerpc/powerpc64/multiarch/stpncpy-power9.S
>
> diff --git a/sysdeps/powerpc/powerpc64/le/power9/stpncpy.S b/sysdeps/powerpc/powerpc64/le/power9/stpncpy.S
> new file mode 100644
> index 0000000000..a96840bb6f
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/le/power9/stpncpy.S
> @@ -0,0 +1,24 @@
> +/* Optimized stpncpy implementation for PowerPC64/POWER9.
> + Copyright (C) 2015-2020 Free Software Foundation, Inc.
Should this date be exclusively 2020?
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#define USE_AS_STPNCPY
> +#include <sysdeps/powerpc/powerpc64/le/power9/strncpy.S>
> +
> +weak_alias (__stpncpy, stpncpy)
> +libc_hidden_def (__stpncpy)
> +libc_hidden_builtin_def (stpncpy)
OK.
> diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
> index cde68384d4..64b06a9040 100644
> --- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
> +++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S
> @@ -18,16 +18,30 @@
>
> #include <sysdep.h>
>
> +#ifdef USE_AS_STPNCPY
> +# ifndef STPNCPY
> +# define FUNC_NAME __stpncpy
> +# else
> +# define FUNC_NAME STPNCPY
> +# endif
> +#else
> # ifndef STRNCPY
> # define FUNC_NAME strncpy
> # else
> # define FUNC_NAME STRNCPY
> # endif
> +#endif /* !USE_AS_STPNCPY */
>
> /* Implements the function
>
> char * [r3] strncpy (char *dest [r3], const char *src [r4], size_t n [r5])
>
> + or
> +
> + char * [r3] stpncpy (char *dest [r3], const char *src [r4], size_t n [r5])
> +
> + if USE_AS_STPNCPY is defined.
> +
> The implementation can load bytes past a null terminator, but only
> up to the next 16B boundary, so it never crosses a page. */
>
> @@ -47,6 +61,13 @@ ENTRY_TOCLESS (FUNC_NAME, 4)
> beq L(zero_padding_loop)
>
> cmpwi r5,0
> +#ifdef USE_AS_STPNCPY
> + bgt L(cont)
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
"Compute pointer to last byte copied into dest." Likwise for the other
copied instances.
> + addi r3,r3,1
> + blr
> +#endif
OK.
> beqlr
This is unreachable in stpncpy, can this be conditionally included in
the !stpncpy configuration?
>
> L(cont):
> @@ -77,12 +98,22 @@ L(cont):
> sldi r10,r5,56 /* stxvl wants size in top 8 bits */
> stxvl 32+v0,r11,r10 /* Partial store */
>
> +#ifdef USE_AS_STPNCPY
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
> + add r3,r11,r5
> +#endif
> blr
>
> L(null):
> sldi r10,r8,56 /* stxvl wants size in top 8 bits */
> stxvl 32+v0,r11,r10 /* Partial store */
>
> +#ifdef USE_AS_STPNCPY
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
> + add r3,r11,r7
> +#endif
> add r11,r11,r8
> sub r5,r5,r8
> b L(zero_padding_loop)
> @@ -164,6 +195,11 @@ L(n_tail4):
> sldi r10,r5,56 /* stxvl wants size in top 8 bits */
> addi r11,r11,48 /* Offset */
> stxvl 32+v3,r11,r10 /* Partial store */
> +#ifdef USE_AS_STPNCPY
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
> + add r3,r11,r5
> +#endif
> blr
>
> L(prep_n_tail1):
> @@ -174,6 +210,11 @@ L(prep_n_tail1):
> L(n_tail1):
> sldi r10,r5,56 /* stxvl wants size in top 8 bits */
> stxvl 32+v0,r11,r10 /* Partial store */
> +#ifdef USE_AS_STPNCPY
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
> + add r3,r11,r5
> +#endif
> blr
>
> L(prep_n_tail2):
> @@ -186,6 +227,11 @@ L(n_tail2):
> sldi r10,r5,56 /* stxvl wants size in top 8 bits */
> addi r11,r11,16 /* offset */
> stxvl 32+v1,r11,r10 /* Partial store */
> +#ifdef USE_AS_STPNCPY
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
> + add r3,r11,r5
> +#endif
> blr
>
> L(prep_n_tail3):
> @@ -199,6 +245,11 @@ L(n_tail3):
> sldi r10,r5,56 /* stxvl wants size in top 8 bits */
> addi r11,r11,32 /* Offset */
> stxvl 32+v2,r11,r10 /* Partial store */
> +#ifdef USE_AS_STPNCPY
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
> + add r3,r11,r5
> +#endif
> blr
>
> L(prep_tail1):
> @@ -208,6 +259,11 @@ L(tail1):
> addi r9,r8,1 /* Add null terminator */
> sldi r10,r9,56 /* stxvl wants size in top 8 bits */
> stxvl 32+v0,r11,r10 /* Partial store */
> +#ifdef USE_AS_STPNCPY
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
> + add r3,r11,r8
> +#endif
> add r11,r11,r9
> sub r5,r5,r9
> b L(zero_padding_loop)
> @@ -222,6 +278,11 @@ L(tail2):
> sldi r10,r9,56 /* stxvl wants size in top 8 bits */
> addi r11,r11,16 /* offset */
> stxvl 32+v1,r11,r10 /* Partial store */
> +#ifdef USE_AS_STPNCPY
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
> + add r3,r11,r8
> +#endif
> add r11,r11,r9
> sub r5,r5,r9
> b L(zero_padding_loop)
> @@ -237,6 +298,11 @@ L(tail3):
> sldi r10,r9,56 /* stxvl wants size in top 8 bits */
> addi r11,r11,32 /* offset */
> stxvl 32+v2,r11,r10 /* Partial store */
> +#ifdef USE_AS_STPNCPY
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
> + add r3,r11,r8
> +#endif
> add r11,r11,r9
> sub r5,r5,r9
> b L(zero_padding_loop)
> @@ -252,6 +318,11 @@ L(tail4):
> sldi r10,r9,56 /* stxvl wants size in top 8 bits */
> addi r11,r11,48 /* offset */
> stxvl 32+v3,r11,r10 /* Partial store */
> +#ifdef USE_AS_STPNCPY
> + /* stpncpy returns the dest address plus the size not counting the
> + final '\0'. */
> + add r3,r11,r8
> +#endif
> add r11,r11,r9
> sub r5,r5,r9
>
> @@ -274,3 +345,6 @@ L(zero_padding_end):
> L(n_tail):
>
> END (FUNC_NAME)
> +#ifndef USE_AS_STPNCPY
> +libc_hidden_builtin_def (strncpy)
> +#endif
OK.
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/Makefile b/sysdeps/powerpc/powerpc64/multiarch/Makefile
> index cd2b47b403..f46bf50732 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/Makefile
> +++ b/sysdeps/powerpc/powerpc64/multiarch/Makefile
OK.
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c b/sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c
> index aa63e1c23f..56790bcfe3 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c
OK.
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/stpncpy-power9.S b/sysdeps/powerpc/powerpc64/multiarch/stpncpy-power9.S
> new file mode 100644
> index 0000000000..ecbbb5c8e9
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/multiarch/stpncpy-power9.S
> @@ -0,0 +1,24 @@
> +/* Optimized stpncpy implementation for POWER9/PPC64.
> + Copyright (C) 2015-2020 Free Software Foundation, Inc.
Minor nit, I suspect that date should only include 2020.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#define STPNCPY __stpncpy_power9
> +
> +#undef libc_hidden_builtin_def
> +#define libc_hidden_builtin_def(name)
> +
OK.
> +#include <sysdeps/powerpc/powerpc64/le/power9/stpncpy.S>
> diff --git a/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c b/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c
> index 17df886431..21702716a3 100644
> --- a/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c
> +++ b/sysdeps/powerpc/powerpc64/multiarch/stpncpy.c
> @@ -26,10 +26,17 @@
> extern __typeof (__stpncpy) __stpncpy_ppc attribute_hidden;
> extern __typeof (__stpncpy) __stpncpy_power7 attribute_hidden;
> extern __typeof (__stpncpy) __stpncpy_power8 attribute_hidden;
> +# ifdef __LITTLE_ENDIAN__
> +extern __typeof (__stpncpy) __stpncpy_power9 attribute_hidden;
> +# endif
> # undef stpncpy
> # undef __stpncpy
>
> libc_ifunc_redirected (__redirect___stpncpy, __stpncpy,
> +# ifdef __LITTLE_ENDIAN__
> + (hwcap2 & PPC_FEATURE2_ARCH_3_00)
> + ? __stpncpy_power9 :
> +# endif
> (hwcap2 & PPC_FEATURE2_ARCH_2_07)
> ? __stpncpy_power8
> : (hwcap & PPC_FEATURE_HAS_VSX)
>
I think the spacing is off by two here.
More information about the Libc-alpha
mailing list