Re: [PATCH 1/2] PPC64: Add libmvec SIMD single-precision natural exponent function.

[Tulio Magno Quites Machado Filho <tuliom at ascii dot art dot br>]

Hi Shawn,

You patch looks very good to me, but there are a couple of things we need to
fix before merging it.

Shawn Landden <shawn@git.icu> writes:

> [BZ #24209]
>
> Passes all tests.
>
> Unlike other libmvec functions, this sets the onderflow and overflow bits.

s/onderflow/underflow/

> The caller can check these flags, and possibly re-run the calculations with
> scalar expf to figure out what is causing the overflow or underflow.
>
> Suprisingly the special-case path performs as well as the normal path.
> (both of which are vectorized)
> Running 20 times over 32MiB
> vector: mean 432.263032 MiB/s (sd 0.486733)
> scalar: mean 178.646197 MiB/s (sd 0.050013)
>
> 2019-05-11 Shawn Landden <shawn@git.icu>

2 spaces ---^-------------^

>         * NEWS: Noted the addition of PPC64 vector expf function.

Indent this line (and the following) with a tab.

More information about the GNU ChangeLog format:
https://sourceware.org/glibc/wiki/Contribution%20checklist#Properly_Formatted_GNU_ChangeLog

> diff --git a/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_math_errf.c b/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_math_errf.c
> new file mode 100644
> index 0000000000..cf3466c400
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_math_errf.c
> @@ -0,0 +1,39 @@
> +/* Single-precision math error handling.

s/math/vector math/

> +   Copyright (C) 2017-2019 Free Software Foundation, Inc.

Copyright 2019.

> diff --git a/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_s_expf4_vsx.c b/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_s_expf4_vsx.c
> new file mode 100644
> index 0000000000..cd04f5d8b3
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_s_expf4_vsx.c
> @@ -0,0 +1,141 @@
> +/* Single-precision vector expf(x) function.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   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
> +   <http://www.gnu.org/licenses/>.  */
> +#include <altivec.h>
> +#include <math.h>
> +
> +#include "math_config_flt.h"
> +
> +typedef vector long long unsigned v64u;
> +typedef union {
> +	vector unsigned u;

This file does not have the correct indentation, e.g. this line should have
been indented with 2 spaces.
If you ever get 8 spaces together, they should be replaced with a tab.

It also has many lines that are too long (80 characters or more).

> +vector float
> +_ZGVbN4v_expf (vector float x)
> +{
> +	u res;
> +	u xu;
> +	xu.f = x;
> +	us c88;
> +	c88.f = 88.0f;
> +	us inf;
> +	inf.f = INFINITY;
> +	us ninf;
> +	ninf.f = -INFINITY;
> +	vector unsigned constants = {(c88.u & 0xfff00000) << 1, ninf.u, inf.u, 0};
> +	vector float constants2 = {0x1.62e42ep6f, -0x1.9fe368p6f, 0, 0};
> +	vector unsigned zero = {0, 0, 0, 0};
> +	vector unsigned v88 = vec_splat (constants, 0);
> +	vector unsigned is_special_case = (vector unsigned) vec_cmpge (xu.u << 1, v88);

This line is too long.

> +	vector double xl = vec_unpackh (x);
> +	vector double xr = vec_unpackl (x);

These built-ins (returning vector double) are only available on GCC >= 8.
Meanwhile, glibc supports GCC >= 6.2.

Thanks!

-- 
Tulio Magno