diff mbox series

[1/2] util/bufferiszero: Use __attribute__((target)) for avx2/avx512

Message ID 20221204015123.362726-2-richard.henderson@linaro.org
State Superseded
Headers show
Series Use a more portable way to enable target specific functions | expand

Commit Message

Richard Henderson Dec. 4, 2022, 1:51 a.m. UTC
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.henderson@linaro.org>
---
 meson.build         |  8 ++------
 util/bufferiszero.c | 41 ++++++-----------------------------------
 2 files changed, 8 insertions(+), 41 deletions(-)

Comments

Daniel P. Berrangé Dec. 5, 2022, 11:17 a.m. UTC | #1
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.henderson@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
Richard Henderson Dec. 5, 2022, 3:16 p.m. UTC | #2
On 12/5/22 05:17, Daniel P. Berrangé wrote:
> 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.henderson@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.

Adding

     Include only <immintrin.h> as that is the outermost "official"
     header for these intrinsics -- emmintrin.h and smmintrin.> are
     older SSE2 and SSE4 specific headers, while the immintrin.h
     includes all of the Intel intrinsics.


r~
diff mbox series

Patch

diff --git a/meson.build b/meson.build
index 5c6b5a1c75..11b873f911 100644
--- a/meson.build
+++ b/meson.build
@@ -2324,11 +2324,9 @@  config_host_data.set('CONFIG_CPUID_H', have_cpuid_h)
 config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \
   .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable AVX2') \
   .require(cc.links('''
-    #pragma GCC push_options
-    #pragma GCC target("avx2")
     #include <cpuid.h>
     #include <immintrin.h>
-    static int bar(void *a) {
+    static int __attribute__((target("avx2"))) bar(void *a) {
       __m256i x = *(__m256i *)a;
       return _mm256_testz_si256(x, x);
     }
@@ -2338,11 +2336,9 @@  config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \
 config_host_data.set('CONFIG_AVX512F_OPT', get_option('avx512f') \
   .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable AVX512F') \
   .require(cc.links('''
-    #pragma GCC push_options
-    #pragma GCC target("avx512f")
     #include <cpuid.h>
     #include <immintrin.h>
-    static int bar(void *a) {
+    static int __attribute__((target("avx512f"))) bar(void *a) {
       __m512i x = *(__m512i *)a;
       return _mm512_test_epi64_mask(x, x);
     }
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>
+#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