Re: [PATCH v2 0/8] centralize AVX-512 feature detection

[David Marchand <david.marchand@redhat.com>]

On Tue, Oct 8, 2024 at 12:03 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Oct 08, 2024 at 10:49:39AM +0200, David Marchand wrote:
> > On Tue, Oct 1, 2024 at 1:19 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > The meson code to detect CPU and compiler support for AVX512 was duplicated
> > > across multiple drivers. Do all detection in just a single place to simplify
> > > the code.
> > >
> > > v2: ensure that target_has_avx512 is always defined on x86 to fix build errors
> > >
> > > Bruce Richardson (8):
> > >   config/x86: add global defines for checking AVX-512
> > >   event/dlb2: use global AVX-512 variables
> > >   common/idpf: use global AVX-512 variables
> > >   net/cpfl: use global AVX-512 variables
> > >   net/i40e: use global AVX-512 variables
> > >   net/iavf: use global AVX-512 variables
> > >   net/ice: use global AVX-512 variables
> > >   net/idpf: use global AVX-512 variables
> > >
> > >  config/x86/meson.build          | 19 +++++++++++----
> > >  drivers/common/idpf/meson.build | 17 ++-----------
> > >  drivers/event/dlb2/meson.build  | 42 +++++++--------------------------
> > >  drivers/net/cpfl/meson.build    | 19 ++-------------
> > >  drivers/net/i40e/meson.build    | 13 ++--------
> > >  drivers/net/iavf/meson.build    | 13 ++--------
> > >  drivers/net/ice/meson.build     | 15 ++----------
> > >  drivers/net/idpf/meson.build    | 19 ++-------------
> > >  8 files changed, 36 insertions(+), 121 deletions(-)
> >
> > Thanks for this cleanup, I have two comments.
> >
> > - Some drivers were going into great lenghts to check that individiual
> > avx512 features were available.
> > With this series, we end up requiring support for all features to
> > announce avx512 availability.
> > Are we perhaps disabling AVX512 support with some toolchains, out
> > there, supporting only part of the set?
> >
>
> The various AVX-512 feature sets checked for (F, BW, VL, DQ) were all
> introduced in the same hardware generation - all are available in gcc when
> using -march=skylake-avx512 or later, or -march=znver4. On the toolchain
> side, gcc introduced all these flags simultaneously in gcc-6 [1]. For
> clang/llvm, testing with godbolt for compiler errors/warnings indicates
> that all these 4 avx512 flags are available from clang 3.6 - the minimum we
> support in DPDK [2]
>
> [1] https://gcc.gnu.org/gcc-6/changes.html
> [2] https://doc.dpdk.org/guides/linux_gsg/sys_reqs.html#compilation-of-the-dpdk

Perfect, thanks for the details.

>
> > - Some drivers were checking for presence of -mno-avx512f in
> > machine_args as a way to disable building any AVX512 stuff.
> > This gets discarded with this series.
> >
>
> Yes, because it should no longer be necessary. The places in the build
> system where we set the no-avx512f flag are reworked so that we don't have
> cc_has_avx512 set.

Ok, it is clearer now.

Last comment on style:
$ git grep cc_avx512_flags drivers/
drivers/common/idpf/meson.build:        avx512_args = [cflags] + cc_avx512_flags
drivers/event/dlb2/meson.build:                               c_args:
cflags + cc_avx512_flags)
drivers/net/i40e/meson.build:        avx512_args = cflags + cc_avx512_flags
drivers/net/iavf/meson.build:        avx512_args = cflags + cc_avx512_flags
drivers/net/ice/meson.build:        avx512_args = cflags + cc_avx512_flags

I think it is safe to remove the [] around cflags in common/idpf, right?


Do you have some cycles to send a v2 and convert lib/net and net/virtio ?
Otherwise, can you do a followup patch for rc2?


Thanks.

-- 
David Marchand