On Sat, Dec 03, 2022 at 07:51:22PM -0600, Richard Henderson wrote:
> Use the attribute, which is supported by clang, instead of
> the #pragma, which is not supported and, for some reason,
> also not detected by the meson probe, so we fail by -Werror.
>
> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
> ---
> meson.build | 8 ++------
> util/bufferiszero.c | 41 ++++++-----------------------------------
> 2 files changed, 8 insertions(+), 41 deletions(-)
>
> diff --git a/util/bufferiszero.c b/util/bufferiszero.c
> index ec3cd4ca15..1790ded7d4 100644
> --- a/util/bufferiszero.c
> +++ b/util/bufferiszero.c
> @@ -64,18 +64,11 @@ buffer_zero_int(const void *buf, size_t len)
> }
>
> #if defined(CONFIG_AVX512F_OPT) || defined(CONFIG_AVX2_OPT) ||
> defined(__SSE2__)
> -/* Do not use push_options pragmas unnecessarily, because clang
> - * does not support them.
> - */
> -#if defined(CONFIG_AVX512F_OPT) || defined(CONFIG_AVX2_OPT)
> -#pragma GCC push_options
> -#pragma GCC target("sse2")
> -#endif
> -#include <emmintrin.h>
So the old code included emmintrin.h, and possibly either
immintrin.h / simmintrin.h, but the new code only
includes immintrin.h.
I'm not saying that's wrong, I'm just wondering why it is
changing, as it feels possibly tangential to the pragma
-> attribute conversion. Could you mention this in the
commit message, or split it to a separate cleanup patch
if its a functionally unrelated change.
> +#include <immintrin.h>
>
> /* Note that each of these vectorized functions require len >= 64. */
>
> -static bool
> +static bool __attribute__((target("sse2")))
> buffer_zero_sse2(const void *buf, size_t len)
> {
> __m128i t = _mm_loadu_si128(buf);
> @@ -104,20 +97,9 @@ buffer_zero_sse2(const void *buf, size_t len)
>
> return _mm_movemask_epi8(_mm_cmpeq_epi8(t, zero)) == 0xFFFF;
> }
> -#if defined(CONFIG_AVX512F_OPT) || defined(CONFIG_AVX2_OPT)
> -#pragma GCC pop_options
> -#endif
>
> #ifdef CONFIG_AVX2_OPT
> -/* Note that due to restrictions/bugs wrt __builtin functions in gcc <= 4.8,
> - * the includes have to be within the corresponding push_options region, and
> - * therefore the regions themselves have to be ordered with increasing ISA.
> - */
> -#pragma GCC push_options
> -#pragma GCC target("sse4")
> -#include <smmintrin.h>
> -
> -static bool
> +static bool __attribute__((target("sse4")))
> buffer_zero_sse4(const void *buf, size_t len)
> {
> __m128i t = _mm_loadu_si128(buf);
> @@ -145,12 +127,7 @@ buffer_zero_sse4(const void *buf, size_t len)
> return _mm_testz_si128(t, t);
> }
>
> -#pragma GCC pop_options
> -#pragma GCC push_options
> -#pragma GCC target("avx2")
> -#include <immintrin.h>
> -
> -static bool
> +static bool __attribute__((target("avx2")))
> buffer_zero_avx2(const void *buf, size_t len)
> {
> /* Begin with an unaligned head of 32 bytes. */
> @@ -176,15 +153,10 @@ buffer_zero_avx2(const void *buf, size_t len)
>
> return _mm256_testz_si256(t, t);
> }
> -#pragma GCC pop_options
> #endif /* CONFIG_AVX2_OPT */
>
> #ifdef CONFIG_AVX512F_OPT
> -#pragma GCC push_options
> -#pragma GCC target("avx512f")
> -#include <immintrin.h>
> -
> -static bool
> +static bool __attribute__((target("avx512f")))
> buffer_zero_avx512(const void *buf, size_t len)
> {
> /* Begin with an unaligned head of 64 bytes. */
> @@ -210,8 +182,7 @@ buffer_zero_avx512(const void *buf, size_t len)
> return !_mm512_test_epi64_mask(t, t);
>
> }
> -#pragma GCC pop_options
> -#endif
> +#endif /* CONFIG_AVX512F_OPT */
>
>
> /* Note that for test_buffer_is_zero_next_accel, the most preferred
> --
> 2.34.1
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
