Re: [PATCH] powerpc: Add multiarch sqrtf128 for ppc64le

[Rajalakshmi Srinivasaraghavan <raji at linux dot vnet dot ibm dot com>]

On 05/29/2018 09:07 PM, Gabriel F. T. Gomes wrote:
> On Thu, 17 May 2018, Rajalakshmi Srinivasaraghavan wrote:
>
> > This patch creates ifunc for sqrtf128() to make use of new xssqrtqp
> > instruction for POWER9 when --enable-multi-arch and --with-cpu=power8
> > options are used on power9 system.  This is achieved by explicitly
> > adding -mcpu=power9 flag for sqrtf128-power9.
> >
> > 2018-05-17  Rajalakshmi Srinivasaraghavan  <raji@linux.vnet.ibm.com>
> >
> > 	* sysdeps/powerpc/powerpc64/le/fpu/multiarch/Makefile: New file to
> > 	add w_sqrtf128-power9 and w_sqrtf128-ppc64le to libm-sysdep_routines.
> > 	* sysdeps/powerpc/powerpc64/le/fpu/multiarch/w_sqrtf128-power9.c:
> > 	New file.
> > 	* sysdeps/powerpc/powerpc64/le/fpu/multiarch/w_sqrtf128-ppc64le.c:
> > 	Likewise.
> > 	* sysdeps/powerpc/powerpc64/le/fpu/multiarch/w_sqrtf128.c: Likewise.
>
> This part looks good to me.

Thanks for the review.

> > 	* sysdeps/powerpc/powerpc64/le/power9/fpu/e_sqrtf128.c: Remove file
> > 	as __ieee754_sqrtf128() for power9 is present already in
> > 	sysdeps/powerpc/fpu/math_private.h.
>
> This doesn't.
>
> The function provided by e_sqrtf128.c (__sqrtf128_finite) is export by
> GLIBC and accessible by users when they are building their code with
> -ffast-math.  When building glibc with --with-cpu=power8, removing this
> file would be OK.  However, when building glibc with --with-cpu=power9,
> __sqrtf128_finite will use software emulation when it could use xssqrtqp.
> Since math_private.h is not installed, it will not work for glibc users.
>
> You patch is OK without this removal.

Good catch. Ack.

> > +++ b/sysdeps/powerpc/powerpc64/le/fpu/multiarch/w_sqrtf128-power9.c
> > (...)
> > +#include <math-type-macros-float128.h>
> > (...)
> > +#undef __USE_WRAPPER_TEMPLATE
> > +#define __USE_WRAPPER_TEMPLATE 1
>
> Do you actually need this?  What behaviour are you trying to override?
> The float128 implementation always uses the wrapper templates.

Yes. This is not needed.

> > +++ b/sysdeps/powerpc/powerpc64/le/fpu/multiarch/w_sqrtf128-ppc64le.c
> > (...)
> > +#include <math-type-macros-float128.h>
> > (...)
> > +#undef __USE_WRAPPER_TEMPLATE
> > +#define __USE_WRAPPER_TEMPLATE 1
>
> Likewise.
>
> > +++ b/sysdeps/powerpc/powerpc64/le/fpu/multiarch/w_sqrtf128.c
> > (...)
> > +libc_ifunc (__sqrtf128,
> > +	    (hwcap2 & PPC_FEATURE2_ARCH_3_00)
> > +	    ? __sqrtf128_power9
> > +	    : __sqrtf128_ppc64le);
> > +declare_mgen_alias (__sqrt, sqrt)
>
> OK.

Pushed as 2c93fce76a8e672abfdf7771c97be9ea49b7222b.

>

--
Thanks
Rajalakshmi S