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

[Tulio Magno Quites Machado Filho <tuliom@ascii.art.br>]

Hi Shawn,

We also have a couple of small issues in this patch as well as 2 testcases
failing.

I'm also raising a concern here based on the copyright of one of the files.
I don't know how to proceed and we'll need help from the project stewards.


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.
>
> The special-case path is not vectorized, and performs much woorse than
> the scalar code.
> Normalized data: 1 to 2^32 converted to double
> Running 20 times over 32MiB
> vector: mean 563.807107 MiB/s (sd 0.390922)
> scalar: mean 226.527824 MiB/s (sd 0.077406)
>
> Random data:
> vector: mean 80.175986 MiB/s (sd 1.110948)
> scalar: mean 244.738130 MiB/s (sd 0.029561)

After applying this patch, I started seeing many errors from
math/test-double-vlen2-exp, e.g.:

testing double (vector length 2)
Failure: Test: exp_vlen2 (qNaN)
Result:
 is:          inf   inf
 should be:  qNaN
Failure: Test: exp_vlen2 (-qNaN)
Result:
 is:          0.0000000000000000e+00   0x0.0000000000000p+0
 should be:  qNaN
Failure: Test: exp_vlen2 (sNaN)
Result:
 is:          inf   inf
 should be:  qNaN
Failure: Test: exp_vlen2 (-sNaN)
Result:
 is:          0.0000000000000000e+00   0x0.0000000000000p+0
 should be:  qNaN
Failure: Test: exp_vlen2 (-0)
Result:
 is:          1.1000000000000001e+00   0x1.199999999999ap+0
 should be:   1.0000000000000000e+00   0x1.0000000000000p+0
 difference:  1.0000000000000009e-01   0x1.99999999999a0p-4
 ulp       :  450359962737050.0000
 max.ulp   :  0.0000
...

I've seen it both with gcc 8.3 and 9.1.

> 2019-05-11 Shawn Landden <shawn@git.icu>
>         * NEWS: Noted the addition of PPC64 vector exp function.

Likewise, this ChangeLog entry needs rework.
Previous comments apply here too.

>         * sysdeps/powerpc/powerpc64/fpu/multiarch/(math_config.h)->(math_config_dbl.h):
> 	Renamed, and modified for exp.

Please use this instead:

	* sysdeps/powerpc/powerpc64/fpu/multiarch/math_config.h: Renamed as...
	* sysdeps/powerpc/powerpc64/fpu/multiarch/math_config_dbl.h: ... and
	modified for exp.

> diff --git a/sysdeps/powerpc/powerpc64/fpu/multiarch/math_config.h b/sysdeps/powerpc/powerpc64/fpu/multiarch/math_config_dbl.h
> similarity index 94%
> rename from sysdeps/powerpc/powerpc64/fpu/multiarch/math_config.h
> rename to sysdeps/powerpc/powerpc64/fpu/multiarch/math_config_dbl.h
> index bd37906ecb..039ff6adeb 100644
> --- a/sysdeps/powerpc/powerpc64/fpu/multiarch/math_config.h
> +++ b/sysdeps/powerpc/powerpc64/fpu/multiarch/math_config_dbl.h
> @@ -17,15 +17,20 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #ifndef _MATH_CONFIG_H
>  #define _MATH_CONFIG_H
>  
> +#ifndef EXP_USE_TOINT_NARROW
> +#define EXP_USE_TOINT_NARROW 0
> +#endif

When is EXP_USE_TOINT_NARROW != 0?
We need source code comments explaining what this macro is doing

> diff --git a/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_d_exp2_vsx.c b/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_d_exp2_vsx.c
> new file mode 100644
> index 0000000000..6a9dfb4492
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_d_exp2_vsx.c
>...
> +
> +/* Top 12 bits of a double (sign and exponent bits).  */
> +static inline uint32_t top12(double x)

Wrong coding style:

static inline uint32_t
top12 (double x)

> +{
> +	return asuint64(x) >> 52;

The indentation style changed from this line forward.

> diff --git a/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_d_exp_data.c b/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_d_exp_data.c
> new file mode 100644
> index 0000000000..650fdee75f
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/fpu/multiarch/vec_d_exp_data.c
> @@ -0,0 +1,200 @@
> +/*
> + * Shared data between exp, exp2 and pow.
> + *
> + * Copyright (c) 2018, Arm Limited.
> + * SPDX-License-Identifier: MIT
> + */

Do you have the approval from the copyright holder to contribute this file?

I don't know if glibc can accept this file as-is.
We need to clarify this.

> diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libmvec.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libmvec.abilist
> index 63770c8da3..5e662ffa70 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libmvec.abilist
> +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libmvec.abilist
> @@ -5,5 +5,6 @@ GLIBC_2.30 _ZGVbN2vvv_sincos F
>  GLIBC_2.30 _ZGVbN4v_cosf F
>  GLIBC_2.30 _ZGVbN4v_logf F
>  GLIBC_2.30 _ZGVbN4v_sinf F
>  GLIBC_2.30 _ZGVbN4vvv_sincosf F
>  GLIBC_2.30 _ZGVbN4v_expf F
> +GLIBC_2.30 _ZGVbN2v_exp F

They have to be properly sorted, unless we get an error from
mathvec/check-abi-libmvec:

@@ -1,0 +2 @@ GLIBC_2.30 _ZGVbN2v_cos F
+GLIBC_2.30 _ZGVbN2v_exp F
@@ -5,0 +7 @@ GLIBC_2.30 _ZGVbN4v_cosf F
+GLIBC_2.30 _ZGVbN4v_expf F
@@ -9,2 +10,0 @@ GLIBC_2.30 _ZGVbN4vvv_sincosf F
-GLIBC_2.30 _ZGVbN4v_expf F
-GLIBC_2.30 _ZGVbN2v_exp F


-- 
Tulio Magno