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

[Richard Henderson]

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~