RE: [PATCH v4] build: reduce use of AVX compiler flags

["Varghese, Vipin" <Vipin.Varghese@amd.com>]

[Public]

Snipped

>
>
> When doing a build for a target that already has the instruction sets for
> AVX2/AVX512 enabled, skip emitting the AVX compiler flags, or the
> skylake-avx512 '-march' flags, as they are unnecessary. Instead, when the default
> flags produce the desired output, just use them unmodified, and don't bother adding
> in extra enabling flags for AVX2 or AVX-512.
>
> Depends-on: series-35006 ("doc/linux_gsg: update recommended compiler
> versions")
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>
> V4: Fix error flagged by CI with clang builds without AVX512 - change
>     "cc_avx512_args" to correct "cc_avx512_flags"
>
> V3: put in version check to work around an issues with some meson
>     versions, (hopefully) allowing builds to pass in all CIs. The
>     printout of the extra flags now only happens with meson >= 0.60.2
>
> V2: dropped the doc update for the minimum compiler version.  Based on
>     discussion, that version bump is larger than proposed in RFC and is
>     now a separate patch/series [series 35006 referenced above]
>
> ---
>  config/x86/meson.build | 31 ++++++++++++++++++++-----------
>  drivers/meson.build    |  9 +--------
>  lib/meson.build        |  9 +--------
>  3 files changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/config/x86/meson.build b/config/x86/meson.build index
> c3564b0011..e6612dbd80 100644
> --- a/config/x86/meson.build
> +++ b/config/x86/meson.build
> @@ -4,11 +4,13 @@
>  if is_ms_compiler
>      cc_avx2_flags = ['/arch:AVX2']
>  else
> -    cc_avx2_flags = ['-mavx2']
> +    cc_avx2_flags = []
> +    if cc.get_define('__AVX2__', args: machine_args) == ''
> +        cc_avx2_flags = ['-mavx2']
> +    endif
>  endif
>
>  cc_has_avx512 = false
> -target_has_avx512 = false
>
>  dpdk_conf.set('RTE_ARCH_X86', 1)
>  if dpdk_conf.get('RTE_ARCH_64')
> @@ -65,26 +67,33 @@ if is_linux or cc.get_id() == 'gcc'
>      endif
>  endif
>
> -cc_avx512_flags = ['-mavx512f', '-mavx512vl', '-mavx512dq', '-mavx512bw', '-
> mavx512cd'] -if (binutils_ok and cc.has_multi_arguments(cc_avx512_flags)
> +avx512_march_flag = '-march=skylake-avx512'
> +cc_avx512_flags = []
> +if (binutils_ok and cc.has_argument(avx512_march_flag)
>          and '-mno-avx512f' not in get_option('c_args'))
>      # check if compiler is working with _mm512_extracti64x4_epi64
>      # Ref: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82887
>      code = '''#include <immintrin.h>
>      void test(__m512i zmm){
>          __m256i ymm = _mm512_extracti64x4_epi64(zmm, 0);}'''
> -    result = cc.compiles(code, args : cc_avx512_flags, name : 'AVX512 checking')
> +    result = cc.compiles(code, args : [avx512_march_flag], name :
> + 'AVX512 checking')
>      if result == false
>          machine_args += '-mno-avx512f'
>          warning('Broken _mm512_extracti64x4_epi64, disabling AVX512 support')
>      else
>          cc_has_avx512 = true
> -        target_has_avx512 = (
> -                cc.get_define('__AVX512F__', args: machine_args) != '' and
> -                cc.get_define('__AVX512BW__', args: machine_args) != '' and
> -                cc.get_define('__AVX512DQ__', args: machine_args) != '' and
> -                cc.get_define('__AVX512VL__', args: machine_args) != ''
> -            )
> +        if cc.get_define('__AVX512F__', args: machine_args) == ''
> +            cc_avx512_flags = [avx512_march_flag]

Hi Bruce, we have reviewed this internally and tested the same. We would like your thought for the following.

- Before patch: we were directly setting AVX512 falgs for F, BW, DQ, VL
- new patch: we are setting the flags for `skylake-server` as bare minimal.
- AMD supports AVX512 from `znver4 and higher`.

As per GCC `https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html`, the extra ISA supported between skylake-server (super set) and znver4 and znver5 are `SAHF, FXSR, XSAVE, RDRND, LZCNT, HLE, PREFETCHW, SGX`.
Currently for DPDK microbenchmarks and examples runs safe as it is not using the `SAHF, FXSR, XSAVE, RDRND, LZCNT, HLE, PREFETCHW, SGX` instructions.

Question: should we check if target is `AMD EPYC` then apply bare minimum as `-march=znver4`, thus avoid possible unsupported instruction generation when non `c_args for march` is passed?

> +            if cc.has_argument('-Wno-overriding-option')
> +                cc_avx512_flags += '-Wno-overriding-option'
> +            endif
> +        endif
> +    endif
> +endif
> +if developer_mode and meson.version().version_compare('>=0.60.2')
> +    message('Extra C flags needed for AVX2 output: @0@'.format(cc_avx2_flags))
> +    if cc_has_avx512
> +        message('Extra C flags needed for AVX512 output:
> +@0@'.format(cc_avx512_flags))
>      endif
>  endif
>
> diff --git a/drivers/meson.build b/drivers/meson.build index
> b0850bbb24..51c824ea40 100644
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -267,18 +267,11 @@ foreach subpath:subdirs
>              endif
>              if sources_avx512.length() > 0 and cc_has_avx512
>                  cflags += '-DCC_AVX512_SUPPORT'
> -                avx512_args = [cflags, cc_avx512_flags]
> -                if not target_has_avx512 and cc.has_argument('-march=skylake-
> avx512')
> -                    avx512_args += '-march=skylake-avx512'
> -                    if cc.has_argument('-Wno-overriding-option')
> -                        avx512_args += '-Wno-overriding-option'
> -                    endif
> -                endif
>                  avx512_lib = static_library(lib_name + '_avx512_lib',
>                          sources_avx512,
>                          dependencies: static_deps,
>                          include_directories: includes,
> -                        c_args: avx512_args)
> +                        c_args: [cflags, cc_avx512_flags])
>                  objs += avx512_lib.extract_objects(sources_avx512)
>              endif
>          endif
> diff --git a/lib/meson.build b/lib/meson.build index 41fd98f4e9..16389b9c34 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -260,18 +260,11 @@ foreach l:libraries
>          endif
>          if sources_avx512.length() > 0 and cc_has_avx512
>              cflags += '-DCC_AVX512_SUPPORT'
> -            avx512_args = [cflags, cflags_avx512, cc_avx512_flags]
> -            if not target_has_avx512 and cc.has_argument('-march=skylake-avx512')
> -                avx512_args += '-march=skylake-avx512'
> -                if cc.has_argument('-Wno-overriding-option')
> -                    avx512_args += '-Wno-overriding-option'
> -                endif
> -            endif
>              avx512_lib = static_library(libname + '_avx512_lib',
>                      sources_avx512,
>                      dependencies: static_deps,
>                      include_directories: includes,
> -                    c_args: avx512_args)
> +                    c_args: [cflags, cflags_avx512, cc_avx512_flags])
>              objs += avx512_lib.extract_objects(sources_avx512)
>          endif
>      endif
> --
> 2.48.1