Re: [RFC] How to add vector math functions to Glibc

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Thu, 9 Oct 2014, Andrew Senkevich wrote:

> lets discuss changes in the testsuite, --enable-mathvec configure
> option and comments for data table.

I think the patch submission needs much more explanation (several 
paragraphs explaining what this patch does and how it relates to previous 
patch submissions and discussion in this area).  At this stage of 
discussion, the carefully written analysis of the implementation choices 
you faced and the decisions you reached, with rationale, is much more 
important than the patch itself.

> diff --git a/configure.ac b/configure.ac
> index 82d0896..c32e508 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -353,6 +353,17 @@ if test "$build_pt_chown" = yes; then
>    AC_DEFINE(HAVE_PT_CHOWN)
>  fi
> 
> +AC_ARG_ENABLE([mathvec],
> +      [AS_HELP_STRING([--enable-mathvec],
> +      [Enable building and installing mathvec @<:@default=yes on
> x86_64 build, else default=no@:>@])],
> +      [build_mathvec=$enableval],
> +      [if test -n "$(gcc -v 2>&1 | grep 'Target: x86_64')"; then :

No, you never put architecture-dependencies in the toplevel configure 
script.  Instead, the default needs to be determined by variables that may 
be set by sysdeps configure fragments.

> diff --git a/math/libm-test.inc b/math/libm-test.inc
> index f86a4fa..39901c4 100644
> --- a/math/libm-test.inc
> +++ b/math/libm-test.inc
> @@ -706,13 +706,15 @@ test_single_errno (const char *test_name, int errno_value,
>  static void
>  test_errno (const char *test_name, int errno_value, int exceptions)
>  {
> -  ++noErrnoTests;
> -  if (exceptions & ERRNO_UNCHANGED)
> -    test_single_errno (test_name, errno_value, 0, "unchanged");
> -  if (exceptions & ERRNO_EDOM)
> -    test_single_errno (test_name, errno_value, EDOM, "EDOM");
> -  if (exceptions & ERRNO_ERANGE)
> -    test_single_errno (test_name, errno_value, ERANGE, "ERANGE");
> +#ifndef TEST_MATHVEC

It would seem better to change test_single_errno where it already has a 
conditional "#ifndef TEST_INLINE".

> +/* Run tests for a given function in TONEAREST rounding modes.  */
> +#define TN_RM_TEST(FUNC, EXACT, ARRAY, LOOP_MACRO, END_MACRO, ...) \

I think you should arrange for IF_ROUND_INIT_* to return false for modes 
other than FE_TONEAREST when doing the vector tests, rather than having a 
new macro like this.

> @@ -6258,7 +6274,11 @@ static const struct test_f_f_data cos_test_data[] =
>  static void
>  cos_test (void)
>  {
> +#ifndef TEST_MATHVEC
>    ALL_RM_TEST (cos, 0, cos_test_data, RUN_TEST_LOOP_f_f, END);
> +#else
> +  TN_RM_TEST (vector_cos, 0, cos_test_data, RUN_TEST_LOOP_f_f, END);
> +#endif
>  }

And I don't think we want conditionals like this for every function - 
indeed, the tests shouldn't need to know which functions have vector 
versions at all.

Instead, I suggest that all the testing of different variants takes place 
in the math/ directory - and in addition to testing float, double, 
ldouble, ifloat, idouble, ildoubl, that there are also cases float-vector, 
double-vector, ldouble-vector.  (I also suggest renaming the ifloat, 
idouble, ildoubl cases to match this general pattern.)

That is, there are some number of variants that may be tested for each 
floating-point type.  It may be useful for sysdeps Makefile fragments to 
be able to add to the list of variants.  math/Makefile should then arrange 
for the tests to be run for all relevant combinations of (type, variant).

> +CFLAGS-test-vec-double.c = -fno-inline -ffloat-store -fno-builtin
> -frounding-math -mavx2 -Wno-unused-function

Again, nothing architecture-specific (such as -mavx) in 
architecture-independent files.  If architecture-specific options are 
needed for testing, you need to set up a system of variables that can go 
in sysdeps Makefile fragments.  And since you can't determine at configure 
time what host the tests might run on, instruction set features such as 
AVX need testing for at runtime (this means building separate source files 
for the test with separate options so that you know the compiler won't 
generate AVX code before you've tested for AVX availability).

> +#include <immintrin.h>
> +
> +extern __m256d _ZGVdN4v_cos(__m256d);

We need an architecture-independent way of testing.  It might involve 
architecture-specific files providing information about how to map from 
the scalar function to the vector function, what vector functions are 
available, etc. - but the structure needs to have such a division between 
architecture-specific and architecture-independent files.

(I'd like tests to cover normal use via the installed headers, such as 
-fopenmp, but I think testing the vector functions directly *is* a good 
idea as well.)

> +/* General constants:
> + * lAbsMask
> + */

I really don't think these comments are sufficient to explain the 
semantics of the values.  I'm expecting comments of the form "the 
following N 64-bit values are IEEE binary64 constants a[0], a[1], ... for 
a minimax polynomial expansion a[0] + a[1]x + a[2]x^2 + ... of 
cos(x+0.125) for x in the interval [0.125,0.25]" or similar - an 
unambiguous description of exactly what the values mean / how they are 
used.  And see my previous point about defining macros for the offsets in 
this table in such a way that build errors will occur if the macro values 
are wrong.

-- 
Joseph S. Myers
joseph@codesourcery.com