Re: [PATCH 9/N v2] [x86_64] Vectorized math functions

[Joseph Myers <joseph at codesourcery dot com>]

On Tue, 2 Dec 2014, Andrew Senkevich wrote:

> +$(objpfx)test-double-vlen2: $(common-objpfx)mathvec/libmvec.so \
> +    $(objpfx)init-arch.o
> +$(objpfx)test-double-vlen4: $(common-objpfx)mathvec/libmvec.so \
> +    $(objpfx)init-arch.o

Depending on $(common-objpfx)mathvec/libmvec.so would break testing 
--disable-shared (it may be broken anyway, but there's no need to add more 
breakage).  It's better to add a variable libmvec to Makeconfig like the 
existing libm variable, then use it everywhere you have a test using this 
library.  Abstracting out paths to libraries like that may also make it 
easier to support testing installed glibc in future.

> +CFLAGS-test-double-vlen2.c = -fno-inline -ffloat-store -fno-builtin
> -frounding-math \
> +     -D__FAST_MATH__ -DTEST_FAST_MATH -D_OPENMP=201307 \
> +     -Wno-unknown-pragmas
> +CFLAGS-test-double-vlen4.c = -fno-inline -ffloat-store -fno-builtin
> -frounding-math \
> +     -D__FAST_MATH__ -DTEST_FAST_MATH -D_OPENMP=201307 \
> +     -Wno-unknown-pragmas $(arch-ext-cflags)

Why is $(arch-ext-cflags) only in one of these two settings?  Even if you 
don't in fact need those flags for x86_64, this is a generic file, so it 
would seem appropriate to be consistent here.

I think some refactoring is needed to reduce duplication of such settings, 
here and in sysdeps makefiles.

* First a preliminary patch could eliminate existing references to 
-frounding-math other than the global one in Makeconfig - with that global 
one, there's no need for it to be specified for individual files as well.

* Then the existing flag settings could be refactored so that settings 
used for groups of related tests use a single variable rather than being 
duplicated in lots of CFLAGS-* variables.

* Then you might have a libm-test-vector-cflags variable (for example), 
that's based on a common libm-test-fast-math-cflags variable (for 
example), also used for the ifloat.c etc. tests, and adds just the 
vector-test-specific settings such as -D_OPENMP=201307.  Both 
CFLAGS-test-double-vlen2.c and CFLAGS-test-double-vlen4.c could just be 
set to $(libm-test-vector-cflags), as would 
CFLAGS-test-double-vlen4-avx2.c in the sysdeps makefile.

> +#define TEST_VEC_LOOP(len) \
> +  do \
> +    { \
> +      for (i=1; i<len; i++) \
> +        { \
> +          if (((FLOAT *) &mr)[0] != ((FLOAT *) &mr)[i]) \
> +            { \
> +              return ((FLOAT *) &mr)[0]+0.1; \
> +            } \
> +        } \
> +      return ((FLOAT *) &mr)[0]; \
> +    } \
> +  while (0)

Spaces around binary operators ('=', '<', '+').  Put this function in a 
common header rather than duplicating it in test-double-vlen2.h and 
test-double-vlen4.h.

> +  TEST_VEC_LOOP(2); \

Space before '('.

> +  TEST_VEC_LOOP(4); \

Likewise.

> +  TEST_VEC_LOOP(4); \

Likewise.

-- 
Joseph S. Myers
joseph@codesourcery.com