Re: [PATCH v3 07/11] acl: use common AVX build handling

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Mar 19, 2025 at 11:40:19AM +0100, David Marchand wrote:
> On Wed, Mar 19, 2025 at 11:26 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 11:16:09AM +0100, David Marchand wrote:
> > > On Tue, Mar 18, 2025 at 6:35 PM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > remove custom logic for building AVX2 and AVX-512 files.
> > > >
> > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> ---
> > > > lib/acl/meson.build | 54
> > > > ++++----------------------------------------- 1 file changed, 4
> > > > insertions(+), 50 deletions(-)
> > > >
> > > > diff --git a/lib/acl/meson.build b/lib/acl/meson.build index
> > > > a80c172812..87e9f25f8e 100644 --- a/lib/acl/meson.build +++
> > > > b/lib/acl/meson.build @@ -15,57 +15,11 @@ headers =
> > > > files('rte_acl.h', 'rte_acl_osdep.h')
> > > >
> > > >  if dpdk_conf.has('RTE_ARCH_X86') sources += files('acl_run_sse.c')
> > > >  - -    avx2_tmplib = static_library('avx2_tmp', -
> > > >  'acl_run_avx2.c', -            dependencies: static_rte_eal, -
> > > >  c_args: [cflags, cc_avx2_flags]) -    objs +=
> > > >  avx2_tmplib.extract_objects('acl_run_avx2.c') - -    # compile
> > > >  AVX512 version if: -    # we are building 64-bit binary AND
> > > >  binutils can generate proper code - -    if
> > > >  dpdk_conf.has('RTE_ARCH_X86_64') and binutils_ok - -        #
> > > >  compile AVX512 version if either: -        # a. we have AVX512
> > > >  supported in minimum instruction set -        #    baseline -
> > > >  # b. it's not minimum instruction set, but supported by -        #
> > > >  compiler -        # -        # in former case, just add avx512 C
> > > >  file to files list -        # in latter case, compile c file to
> > > >  static lib, using correct -        # compiler flags, and then have
> > > >  the .o file from static lib -        # linked into main lib.  - -
> > > >  # check if all required flags already enabled (variant a).  -
> > > >  acl_avx512_flags = ['__AVX512F__', '__AVX512VL__', -
> > > >  '__AVX512CD__', '__AVX512BW__']
> > >
> > > Not sure it is an issue.. CD is not part of common cc_avx512_flags.
> > >
> > It is since bce754b5d942 ("config/x86: add more flags in common AVX512
> > flags set") See:
> > https://github.com/DPDK/dpdk/blob/main/config/x86/meson.build#L68
> 
> Err.. I meant the target_has_avx512 variable.  But looking again, it
> seems we can remove this variable entirely after the series.
> 
Yes, though I'm now thinking that we may actually want to use it.

Since we have the avx handling "centralised" it might be worthwhile
looking again at how we might generate the most efficient code in all
cases. In some cases, using the AVX512 flag explicitly, along with
march=skylake-avx512 flag, may produce worse code than if we just used e.g.
the "march=native" flag. While this code was spread across a dozen files,
and newer instruction sets beyond AVX512 were not that many/common, keeping
checks short and to a minimum makes sense, but now it's centralised in only
2 files and new CPU generations add more capabilities, an extra check or
two doesn't seem so bad.

/Bruce