This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] tst-audit4, tst-audit10: Compile AVX/AVX-512 code separately [BZ #19269]
- From: "Carlos O'Donell" <codonell at redhat dot com>
- To: Florian Weimer <fweimer at redhat dot com>, GNU C Library <libc-alpha at sourceware dot org>
- Date: Mon, 7 Mar 2016 10:12:11 -0500
- Subject: Re: [PATCH] tst-audit4, tst-audit10: Compile AVX/AVX-512 code separately [BZ #19269]
- Authentication-results: sourceware.org; auth=none
- References: <56DD9703 dot 3060303 at redhat dot com>
On 03/07/2016 09:58 AM, Florian Weimer wrote:
> This ensures that GCC will not use unsupported instructions before the
> run-time check to ensure support.
>
> According to the GCC documentation, "avx" and "avx512f" are not
> currently supported as function target attributes, so this seems the
> most conservative fix.
Thanks for fixing this.
> 0001-tst-audit4-tst-audit10-Compile-AVX-AVX-512-code-sepa.patch
>
>
> 2016-03-07 Florian Weimer <fweimer@redhat.com>
>
> [BZ #19269]
> * sysdeps/x86_64/Makefile (tst-audit4): Depend on
> tst-audit4-aux.o.
> (tst-audit10): Depend on tst-audit10-aux.o.
> (CFLAGS-tst-audit4-aux.c): Compile with AVX enabled.
> (CFLAGS-tst-audit10-aux.c): Compile with AVX512 enabled.
> * sysdeps/x86_64/tst-audit4.c (do_test): Call tst_audit4_aux
> instead of inline AVX code.
> * sysdeps/x86_64/tst-audit10.c (do_test): Call tst_audit10_aux
> instead of inline AVX512 code.
> * sysdeps/x86_64/tst-audit4-aux.c: New file
> * sysdeps/x86_64/tst-audit10-aux.c: New file
This looks perfect to me. I like the solution of splitting out the avx-related
pieces into their own distinct source file to avoid any leakage (though one
day we'll face a reckoning of epic proportions when LTO arrives).
Thanks for using the underused feature of error code 77 for dynamically
reporting an unsupported test. We could probably codify that a bit better in
test-skeleton.c so we don't use the constant directly, but that's orthogonal
cleanup.
Please check this in.
> diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
> index 788e4fc..aa4a754 100644
> --- a/sysdeps/x86_64/Makefile
> +++ b/sysdeps/x86_64/Makefile
> @@ -64,7 +64,7 @@ $(objpfx)tst-audit3: $(objpfx)tst-auditmod3a.so
> $(objpfx)tst-audit3.out: $(objpfx)tst-auditmod3b.so
> tst-audit3-ENV = LD_AUDIT=$(objpfx)tst-auditmod3b.so
>
> -$(objpfx)tst-audit4: $(objpfx)tst-auditmod4a.so
> +$(objpfx)tst-audit4: $(objpfx)tst-audit4-aux.o $(objpfx)tst-auditmod4a.so
OK.
> $(objpfx)tst-audit4.out: $(objpfx)tst-auditmod4b.so
> tst-audit4-ENV = LD_AUDIT=$(objpfx)tst-auditmod4b.so
>
> @@ -81,12 +81,12 @@ $(objpfx)tst-audit7: $(objpfx)tst-auditmod7a.so
> $(objpfx)tst-audit7.out: $(objpfx)tst-auditmod7b.so
> tst-audit7-ENV = LD_AUDIT=$(objpfx)tst-auditmod7b.so
>
> -$(objpfx)tst-audit10: $(objpfx)tst-auditmod10a.so
> +$(objpfx)tst-audit10: $(objpfx)tst-audit10-aux.o $(objpfx)tst-auditmod10a.so
> $(objpfx)tst-audit10.out: $(objpfx)tst-auditmod10b.so
OK.
> tst-audit10-ENV = LD_AUDIT=$(objpfx)tst-auditmod10b.so
>
> AVX-CFLAGS=-mavx -mno-vzeroupper
> -CFLAGS-tst-audit4.c += $(AVX-CFLAGS)
> +CFLAGS-tst-audit4-aux.c += $(AVX-CFLAGS)
OK.
> CFLAGS-tst-auditmod4a.c += $(AVX-CFLAGS)
> CFLAGS-tst-auditmod4b.c += $(AVX-CFLAGS)
> CFLAGS-tst-auditmod6b.c += $(AVX-CFLAGS)
> @@ -94,7 +94,7 @@ CFLAGS-tst-auditmod6c.c += $(AVX-CFLAGS)
> CFLAGS-tst-auditmod7b.c += $(AVX-CFLAGS)
> ifeq (yes,$(config-cflags-avx512))
> AVX512-CFLAGS = -mavx512f
> -CFLAGS-tst-audit10.c += $(AVX512-CFLAGS)
> +CFLAGS-tst-audit10-aux.c += $(AVX512-CFLAGS)
OK.
> CFLAGS-tst-auditmod10a.c += $(AVX512-CFLAGS)
> CFLAGS-tst-auditmod10b.c += $(AVX512-CFLAGS)
> endif
> diff --git a/sysdeps/x86_64/tst-audit10-aux.c b/sysdeps/x86_64/tst-audit10-aux.c
> new file mode 100644
> index 0000000..4398b8f
> --- /dev/null
> +++ b/sysdeps/x86_64/tst-audit10-aux.c
> @@ -0,0 +1,41 @@
> +/* Test case for preserved AVX512 registers in dynamic linker, -mavx512f part.
> + Copyright (C) 2012-2016 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <immintrin.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +extern __m512i audit_test (__m512i, __m512i, __m512i, __m512i,
> + __m512i, __m512i, __m512i, __m512i);
> +
> +int
> +tst_audit10_aux (void)
> +{
> +#ifdef __AVX512F__
> + __m512i zmm = _mm512_setzero_si512 ();
> + __m512i ret = audit_test (zmm, zmm, zmm, zmm, zmm, zmm, zmm, zmm);
> +
> + zmm = _mm512_set1_epi64 (0x12349876);
> +
> + if (memcmp (&zmm, &ret, sizeof (ret)))
> + abort ();
> + return 0;
> +#else /* __AVX512F__ */
> + return 77;
> +#endif /* __AVX512F__ */
> +}
OK. It makes the most sense to split these out and thus avoid any leakage
of avx instructions. This looks good.
> diff --git a/sysdeps/x86_64/tst-audit10.c b/sysdeps/x86_64/tst-audit10.c
> index d104341..92e0cb4 100644
> --- a/sysdeps/x86_64/tst-audit10.c
> +++ b/sysdeps/x86_64/tst-audit10.c
> @@ -1,4 +1,5 @@
> -/* Copyright (C) 2012-2016 Free Software Foundation, Inc.
> +/* Test case for preserved AVX512 registers in dynamic linker.
> + Copyright (C) 2012-2016 Free Software Foundation, Inc.
> This file is part of the GNU C Library.
>
> The GNU C Library is free software; you can redistribute it and/or
> @@ -15,13 +16,9 @@
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
>
> -/* Test case for x86-64 preserved registers in dynamic linker. */
> -
> -#ifdef __AVX512F__
> -#include <stdlib.h>
> -#include <string.h>
> #include <cpuid.h>
> -#include <immintrin.h>
> +
> +int tst_audit10_aux (void);
>
> static int
> avx512_enabled (void)
> @@ -42,32 +39,15 @@ avx512_enabled (void)
> return (eax & 0xe6) == 0xe6;
> }
>
> -
> -extern __m512i audit_test (__m512i, __m512i, __m512i, __m512i,
> - __m512i, __m512i, __m512i, __m512i);
> static int
> do_test (void)
> {
> /* Run AVX512 test only if AVX512 is supported. */
> if (avx512_enabled ())
> - {
> - __m512i zmm = _mm512_setzero_si512 ();
> - __m512i ret = audit_test (zmm, zmm, zmm, zmm, zmm, zmm, zmm, zmm);
> -
> - zmm = _mm512_set1_epi64 (0x12349876);
> -
> - if (memcmp (&zmm, &ret, sizeof (ret)))
> - abort ();
> - }
> - return 0;
> -}
> -#else
> -static int
> -do_test (void)
> -{
> - return 0;
> + return tst_audit10_aux ();
> + else
> + return 77;
> }
> -#endif
OK.
>
> #define TEST_FUNCTION do_test ()
> #include "../../test-skeleton.c"
> diff --git a/sysdeps/x86_64/tst-audit4-aux.c b/sysdeps/x86_64/tst-audit4-aux.c
> new file mode 100644
> index 0000000..a1aeb65
> --- /dev/null
> +++ b/sysdeps/x86_64/tst-audit4-aux.c
> @@ -0,0 +1,39 @@
> +/* Test case for preserved AVX registers in dynamic linker, -mavx part.
> + Copyright (C) 2009-2016 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <immintrin.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +extern __m256i audit_test (__m256i, __m256i, __m256i, __m256i,
> + __m256i, __m256i, __m256i, __m256i);
> +
> +int
> +tst_audit4_aux (void)
> +{
> +#ifdef __AVX__
> + __m256i ymm = _mm256_setzero_si256 ();
> + __m256i ret = audit_test (ymm, ymm, ymm, ymm, ymm, ymm, ymm, ymm);
> + ymm = _mm256_set1_epi32 (0x12349876);
> + if (memcmp (&ymm, &ret, sizeof (ret)))
> + abort ();
> + return 0;
> +#else /* __AVX__ */
> + return 77;
Good. Marks the test as UNSUPPORTED.
> +#endif /* __AVX__ */
> +}
> diff --git a/sysdeps/x86_64/tst-audit4.c b/sysdeps/x86_64/tst-audit4.c
> index 44d5123..d8e2ab1 100644
> --- a/sysdeps/x86_64/tst-audit4.c
> +++ b/sysdeps/x86_64/tst-audit4.c
> @@ -1,11 +1,24 @@
> -/* Test case for x86-64 preserved registers in dynamic linker. */
> +/* Test case for preserved AVX registers in dynamic linker.
> + Copyright (C) 2009-2016 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
>
> -#ifdef __AVX__
> -#include <stdlib.h>
> -#include <string.h>
> #include <cpuid.h>
> -#include <immintrin.h>
>
> +int tst_audit4_aux (void);
>
> static int
> avx_enabled (void)
> @@ -22,31 +35,15 @@ avx_enabled (void)
> return (eax & 6) == 6;
> }
>
> -
> -extern __m256i audit_test (__m256i, __m256i, __m256i, __m256i,
> - __m256i, __m256i, __m256i, __m256i);
> static int
> do_test (void)
> {
> /* Run AVX test only if AVX is supported. */
> if (avx_enabled ())
> - {
> - __m256i ymm = _mm256_setzero_si256 ();
> - __m256i ret = audit_test (ymm, ymm, ymm, ymm, ymm, ymm, ymm, ymm);
> -
> - ymm = _mm256_set1_epi32 (0x12349876);
> - if (memcmp (&ymm, &ret, sizeof (ret)))
> - abort ();
> - }
> - return 0;
> -}
> -#else
> -static int
> -do_test (void)
> -{
> - return 0;
> + return tst_audit4_aux ();
> + else
> + return 77;
> }
> -#endif
>
> #define TEST_FUNCTION do_test ()
> #include "../../test-skeleton.c"
OK.
Cheers,
Carlos.