This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH x86-64][BZ #20024] Fixed vector sincos/sincosf ABI
- From: Andrew Senkevich <andrew dot n dot senkevich at gmail dot com>
- To: Joseph Myers <joseph at codesourcery dot com>
- Cc: libc-alpha <libc-alpha at sourceware dot org>
- Date: Thu, 30 Jun 2016 15:35:47 +0300
- Subject: Re: [PATCH x86-64][BZ #20024] Fixed vector sincos/sincosf ABI
- Authentication-results: sourceware.org; auth=none
- References: <CAMXFM3tO0RGFjbrQaEV80rTCWmUy8LQjkZ7gRd9j+Q3YQ2+nAQ at mail dot gmail dot com> <alpine dot DEB dot 2 dot 20 dot 1606022247540 dot 9542 at digraph dot polyomino dot org dot uk> <CAMXFM3vREvMPjBU23ZGV6eyNs-exwHGG6C2WE6N1MHf=CqbkvA at mail dot gmail dot com> <alpine dot DEB dot 2 dot 20 dot 1606061404540 dot 15183 at digraph dot polyomino dot org dot uk> <CAMXFM3uHWDzdEmjDk-jX14D55vMr0QbaqAKKGe5KuWVDyXcRmA at mail dot gmail dot com>
2016-06-30 14:43 GMT+03:00 Andrew Senkevich <andrew.n.senkevich@gmail.com>:
> 2016-06-06 17:08 GMT+03:00 Joseph Myers <joseph@codesourcery.com>:
>> On Mon, 6 Jun 2016, Andrew Senkevich wrote:
>>
>>> 2016-06-03 1:50 GMT+03:00 Joseph Myers <joseph@codesourcery.com>:
>>> > On Tue, 31 May 2016, Andrew Senkevich wrote:
>>> >
>>> >> Hi,
>>> >>
>>> >> this patch fixes wrong vector sincos/sincosf ABI to have it compatible with
>>> >> current vector function declaration. According to current vector function
>>> >> declaration vectorized sincos should have vector of pointers for second and
>>> >> third parameters, so it is fixed with implementation as wrapper to version
>>> >> having second and third parameters as pointers.
>>> >> Is it Ok for trunk, 2.22 and 2.23 releases branches?
>>> >
>>> > Do you intend a followup for trunk only that exports the new functions
>>> > with the intended ABI and makes the old ones into compat symbols?
>>>
>>> Is it suitable way to have both simd declarations for sincos in headers?
>>
>> (a) Would that work usefully, and cause both functions to be used
>> depending on the code to be vectorized?
>>
>> (b) How useful are the existing functions, i.e. would real code be likely
>> to use both functions?
>
> It is hard to say about real code, but GCC 6.1 can vectorize both
> cases depending on user code.
>
> So we can have both versions and test them both with the following
> patch (after main patch from this thread):
>
> diff --git a/sysdeps/unix/sysv/linux/x86_64/libmvec.abilist
> b/sysdeps/unix/sysv/linux/x86_64/libmvec.abilist
> index 80d028a..e3e450c 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/libmvec.abilist
> +++ b/sysdeps/unix/sysv/linux/x86_64/libmvec.abilist
> @@ -47,3 +47,12 @@ GLIBC_2.22 _ZGVeN8v_log F
> GLIBC_2.22 _ZGVeN8v_sin F
> GLIBC_2.22 _ZGVeN8vv_pow F
> GLIBC_2.22 _ZGVeN8vvv_sincos F
> +GLIBC_2.24 GLIBC_2.24 A
> +GLIBC_2.24 _ZGVbN2vl8l8_sincos F
> +GLIBC_2.24 _ZGVbN4vl4l4_sincosf F
> +GLIBC_2.24 _ZGVcN4vl8l8_sincos F
> +GLIBC_2.24 _ZGVcN8vl4l4_sincosf F
> +GLIBC_2.24 _ZGVdN4vl8l8_sincos F
> +GLIBC_2.24 _ZGVdN8vl4l4_sincosf F
> +GLIBC_2.24 _ZGVeN16vl4l4_sincosf F
> +GLIBC_2.24 _ZGVeN8vl8l8_sincos F
> diff --git a/sysdeps/x86/fpu/bits/math-vector.h
> b/sysdeps/x86/fpu/bits/math-vector.h
> index ca43cf4..74a6bf8 100644
> --- a/sysdeps/x86/fpu/bits/math-vector.h
> +++ b/sysdeps/x86/fpu/bits/math-vector.h
> @@ -43,9 +43,9 @@
> # undef __DECL_SIMD_sinf
> # define __DECL_SIMD_sinf __DECL_SIMD_x86_64
> # undef __DECL_SIMD_sincos
> -# define __DECL_SIMD_sincos __DECL_SIMD_x86_64
> +# define __DECL_SIMD_sincos __DECL_SIMD_x86_64 _Pragma ("omp declare
> simd notinbranch linear (__sinx, __cosx: 1)")
> # undef __DECL_SIMD_sincosf
> -# define __DECL_SIMD_sincosf __DECL_SIMD_x86_64
> +# define __DECL_SIMD_sincosf __DECL_SIMD_x86_64 _Pragma ("omp
> declare simd notinbranch linear (__sinx, __cosx: 1)")
> # undef __DECL_SIMD_log
> # define __DECL_SIMD_log __DECL_SIMD_x86_64
> # undef __DECL_SIMD_logf
> diff --git a/sysdeps/x86_64/fpu/Makefile b/sysdeps/x86_64/fpu/Makefile
> index 25aef40..ee281c2 100644
> --- a/sysdeps/x86_64/fpu/Makefile
> +++ b/sysdeps/x86_64/fpu/Makefile
> @@ -170,8 +170,8 @@ float-vlen8-arch-ext-cflags = -mavx
> float-vlen8-arch-ext2-cflags = -mavx2
> float-vlen16-arch-ext-cflags = -mavx512f
>
> -libmvec-sincos-cflags = $(libm-test-fast-math-cflags) -fopenmp
> -Wno-unknown-pragmas
> -libmvec-alias-cflags = $(libmvec-sincos-cflags) -fno-inline
> -ffloat-store -ffinite-math-only
> +libmvec-sincos-cflags = $(libm-test-fast-math-cflags) -fopenmp
> -fno-inline -Wno-unknown-pragmas
> +libmvec-alias-cflags = $(libmvec-sincos-cflags) -ffloat-store
> -ffinite-math-only
>
> CFLAGS-test-double-libmvec-alias-mod.c = $(libmvec-alias-cflags)
> CFLAGS-test-double-libmvec-alias-avx-mod.c =
> $(double-vlen4-arch-ext-cflags) $(libmvec-alias-cflags) -DREQUIRE_AVX
> diff --git a/sysdeps/x86_64/fpu/Versions b/sysdeps/x86_64/fpu/Versions
> index 0813204..02df4b5 100644
> --- a/sysdeps/x86_64/fpu/Versions
> +++ b/sysdeps/x86_64/fpu/Versions
> @@ -13,4 +13,8 @@ libmvec {
> _ZGVbN4vv_powf; _ZGVcN8vv_powf; _ZGVdN8vv_powf; _ZGVeN16vv_powf;
> _ZGVbN4vvv_sincosf; _ZGVcN8vvv_sincosf; _ZGVdN8vvv_sincosf;
> _ZGVeN16vvv_sincosf;
> }
> + GLIBC_2.24 {
> + _ZGVbN2vl8l8_sincos; _ZGVcN4vl8l8_sincos; _ZGVdN4vl8l8_sincos;
> _ZGVeN8vl8l8_sincos;
> + _ZGVbN4vl4l4_sincosf; _ZGVcN8vl4l4_sincosf; _ZGVdN8vl4l4_sincosf;
> _ZGVeN16vl4l4_sincosf;
> + }
> }
> diff --git a/sysdeps/x86_64/fpu/test-double-libmvec-sincos.c
> b/sysdeps/x86_64/fpu/test-double-libmvec-sincos.c
> index 80348a2..8fe106d 100644
> --- a/sysdeps/x86_64/fpu/test-double-libmvec-sincos.c
> +++ b/sysdeps/x86_64/fpu/test-double-libmvec-sincos.c
> @@ -21,6 +21,7 @@
>
> #define N 1000
> double x[N], s[N], c[N];
> +double x1[N], s1[N], c1[N];
> double* s_ptrs[N];
> double* c_ptrs[N];
> int arch_check = 1;
> @@ -28,15 +29,13 @@ int arch_check = 1;
> static void
> init_arg (void)
> {
> - int i;
> -
> CHECK_ARCH_EXT;
>
> arch_check = 0;
>
> - for(i = 0; i < N; i++)
> + for(int i = 0; i < N; i++)
> {
> - x[i] = i / 3;
> + x[i] = x1[i] = i / 3;
> s_ptrs[i] = &s[i];
> c_ptrs[i] = &c[i];
> }
> @@ -45,16 +44,19 @@ init_arg (void)
> static int
> test_sincos_abi (void)
> {
> - int i;
> -
> - init_arg ();
> +#pragma omp simd
> + for(int i = 0; i < N; i++)
> + sincos (x[i], s_ptrs[i], c_ptrs[i]);
>
> - if (arch_check)
> - return 77;
> + return 0;
> +}
>
> +static int
> +test_sincos_linear_abi (void)
> +{
> #pragma omp simd
> - for(i = 0; i < N; i++)
> - sincos (x[i], s_ptrs[i], c_ptrs[i]);
> + for(int i = 0; i < N; i++)
> + sincos (x1[i], &s1[i], &c1[i]);
>
> return 0;
> }
> @@ -62,7 +64,16 @@ test_sincos_abi (void)
> static int
> do_test (void)
> {
> - return test_sincos_abi ();
> + init_arg ();
> +
> + if (arch_check)
> + return 77;
> +
> + test_sincos_abi ();
> +
> + test_sincos_linear_abi ();
> +
> + return 0;
> }
>
> #define TEST_FUNCTION do_test ()
>
> (The same change for sysdeps/x86_64/fpu/test-float-libmvec-sincosf.c)
>
> Is this change Ok generally?
More correct change for sysdeps/x86/fpu/bits/math-vector.h is:
diff --git a/sysdeps/x86/fpu/bits/math-vector.h
b/sysdeps/x86/fpu/bits/math-vector.h
index ca43cf4..c20715b
--- a/sysdeps/x86/fpu/bits/math-vector.h
+++ b/sysdeps/x86/fpu/bits/math-vector.h
@@ -28,9 +28,11 @@
# if defined _OPENMP && _OPENMP >= 201307
/* OpenMP case. */
# define __DECL_SIMD_x86_64 _Pragma ("omp declare simd notinbranch")
+# define __DECL_SIMD_x86_64_sincos __DECL_SIMD_x86_64 _Pragma ("omp
declare simd notinbranch linear (__sinx, __cosx: 1)")
# elif __GNUC_PREREQ (6,0)
/* W/o OpenMP use GCC 6.* __attribute__ ((__simd__)). */
# define __DECL_SIMD_x86_64 __attribute__ ((__simd__ ("notinbranch")))
+# define __DECL_SIMD_x86_64_sincos __DECL_SIMD_x86_64
# endif
# ifdef __DECL_SIMD_x86_64
@@ -43,9 +45,9 @@
# undef __DECL_SIMD_sinf
# define __DECL_SIMD_sinf __DECL_SIMD_x86_64
# undef __DECL_SIMD_sincos
-# define __DECL_SIMD_sincos __DECL_SIMD_x86_64
+# define __DECL_SIMD_sincos __DECL_SIMD_x86_64_sincos
# undef __DECL_SIMD_sincosf
-# define __DECL_SIMD_sincosf __DECL_SIMD_x86_64
+# define __DECL_SIMD_sincosf __DECL_SIMD_x86_64_sincos
# undef __DECL_SIMD_log
# define __DECL_SIMD_log __DECL_SIMD_x86_64
# undef __DECL_SIMD_logf
--
WBR,
Andrew