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

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

On Tue, 30 Sep 2014, Andrew Senkevich wrote:

> diff --git a/configure.ac b/configure.ac
> index 82d0896..c5c1758 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -903,7 +903,7 @@ LIBC_PROG_BINUTILS
>  # Accept binutils 2.20 or newer.
>  AC_CHECK_PROG_VER(AS, $AS, --version,
>    [GNU assembler.* \([0-9]*\.[0-9.]*\)],
> -  [2.1[0-9][0-9]*|2.[2-9][0-9]*|[3-9].*|[1-9][0-9]*], AS=:
> critic_missing="$critic_missing as")
> +  [2.1[0-9][0-9]*|2.[2-9][2-9]*|[3-9].*|[1-9][0-9]*], AS=:
> critic_missing="$critic_missing as")
>  AC_CHECK_PROG_VER(LD, $LD, --version,
>    [GNU ld.* \([0-9][0-9]*\.[0-9.]*\)],
>    [2.1[0-9][0-9]*|2.[2-9][0-9]*|[3-9].*|[1-9][0-9]*], LD=:
> critic_missing="$critic_missing ld")

Any change to required versions needs to include an update to install.texi 
(and the generated INSTALL file).  It should also be proposed in a 
separate thread whose subject describes what is being proposed.

> +# We need to install libm.so as linker script
> +# for more comfortable use of vector math library.
> +subdir_install: $(inst_libdir)/libm.so
> +
> +$(inst_libdir)/libm.so: $(common-objpfx)format.lds \
> + $(common-objpfx)math/libm.so$(libm.so-version) \
> + $(common-objpfx)mathvec/libmvec.so$(libmvec.so-version) \
> + $(+force)
> + (echo '/* GNU ld script */';\
> + cat $<; \
> + echo 'GROUP ( $(slibdir)/libm.so$(libm.so-version) ' \
> + 'AS_NEEDED ( $(slibdir)/libmvec.so$(libmvec.so-version) ) )' \
> + ) > $@.new
> + mv -f $@.new $@

Do you have ordering issues here?  It seems bad for math/ to install a 
direct symlink and then mathvec/ to change it to something else - all 
installation rules for libm should be in the math/ directory.

Do you need to link libmvec against libm (and if so, I'd expect associated 
Makefile rules, and maybe a Depend file to ensure the directories are 
built in the right order)?

Also, I'm not sure the empty libmvec option for unsupported architectures 
when we consider the case where functions require GCC or binutils versions 
newer than we wish to require, so they are optional on some architecture.  
I think having libmvec built or not built on that architecture, depending 
on the tools installed, is better than possibly having it built but empty 
if the tools are too old.

> diff --git a/sysdeps/unix/sysv/linux/shlib-versions
> b/sysdeps/unix/sysv/linux/shlib-versions
> index 9160557..4a32c8a 100644
> --- a/sysdeps/unix/sysv/linux/shlib-versions
> +++ b/sysdeps/unix/sysv/linux/shlib-versions
> @@ -1,2 +1,3 @@
>  libm=6
>  libc=6
> +libmvec=1

There is nothing Linux-specific about this library, so the toplevel 
shlib-versions seems better.

Did the patch pass the testsuite?  If so, you have a problem - you didn't 
add ABI test baselines for this library (in this version, a default empty 
baseline, and one in sysdeps/unix/sysv/linux/x86_64), so the ABI tests 
should have failed, and you need to find out why they didn't run for this 
library, and fix that.  If it failed for lack of ABI test baselines, add 
them.

> +#if defined __x86_64__ && defined __FAST_MATH__
> +# define __DECL_SIMD_AVX2
> +# define __DECL_SIMD_SSE4

I don't see the need for this initial define to empty and subsequent 
#undef.  Except you should probably have comments explaining exactly what 
these macros mean in terms of what function versions they define to be 
available.

> +# if defined _OPENMP && _OPENMP >= 201307
> +/* OpenMP case. */
> +#  undef __DECL_SIMD_AVX2
> +#  undef __DECL_SIMD_SSE4
> +#  define __DECL_SIMD_AVX2 _Pragma("omp declare simd notinbranch")
> +#  define __DECL_SIMD_SSE4 _Pragma("omp declare simd notinbranch")

I think there should be a comment pointing to the ABI/API documentation 
that says what function versions this pragma defines to be available and 
guaranteeing that it will not be redefined to e.g. say that AVX512 is 
available so that existing headers will work with future compilers (but 
another pragma will be needed if in future AVX512 versions are added).

> +# elif defined _CILKPLUS && _CILKPLUS >= 0
> +/* CilkPlus case. TODO _CILKPLUS currently nowhere defined */
> +#  undef __DECL_SIMD_AVX2
> +#  undef __DECL_SIMD_SSE4
> +#  define __DECL_SIMD_AVX2 __attribute__((vector (nomask)))
> +#  define __DECL_SIMD_SSE4 __attribute__((vector (processor(core_i7_sse4_2), \
> +  nomask)))

To be namespace-clean, you have to use reserved-namespace versions of 
attributes.  That is, __vector__, __nomask__, __processor__ and 
__core_i7_sse4_2__.

> + .align 64
> + .globl __gnu_svml_dcos_data
> +__gnu_svml_dcos_data:
> + .long 4294967295

What are the semantics of the values in this table (please add a comment)?  
How was this table generated?

> + .type __gnu_svml_dcos_data,@object
> + .size __gnu_svml_dcos_data,1600

.size __gnu_svml_dcos_data,.-__gnu_svml_dcos_data

seems better than hardcoding another magic number for the size here.

-- 
Joseph S. Myers
joseph@codesourcery.com