Re: [dpdk-dev] [PATCH v3 4/8] fib: introduce AVX512 lookup

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

On Tue, May 19, 2020 at 01:12:59PM +0100, Vladimir Medvedkin wrote:
> Add new lookup implementation for DIR24_8 algorithm using
> AVX512 instruction set
> 
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
> ---
>  lib/librte_fib/Makefile         |  14 ++++
>  lib/librte_fib/dir24_8.c        |  24 ++++++
>  lib/librte_fib/dir24_8_avx512.c | 165 ++++++++++++++++++++++++++++++++++++++++
>  lib/librte_fib/dir24_8_avx512.h |  24 ++++++
>  lib/librte_fib/meson.build      |  11 +++
>  lib/librte_fib/rte_fib.h        |   3 +-
>  6 files changed, 240 insertions(+), 1 deletion(-)
>  create mode 100644 lib/librte_fib/dir24_8_avx512.c
>  create mode 100644 lib/librte_fib/dir24_8_avx512.h
>
<snip> 
> diff --git a/lib/librte_fib/meson.build b/lib/librte_fib/meson.build
> index 771828f..0963f3c 100644
> --- a/lib/librte_fib/meson.build
> +++ b/lib/librte_fib/meson.build
> @@ -5,3 +5,14 @@
>  sources = files('rte_fib.c', 'rte_fib6.c', 'dir24_8.c', 'trie.c')
>  headers = files('rte_fib.h', 'rte_fib6.h')
>  deps += ['rib']
> +
> +if dpdk_conf.has('RTE_ARCH_X86') and cc.has_argument('-mavx512f')
> +	if cc.has_argument('-mavx512dq')
> +		dir24_8_avx512_tmp = static_library('dir24_8_avx512_tmp',
> +			'dir24_8_avx512.c',
> +			dependencies: static_rte_eal,
> +			c_args: cflags + ['-mavx512f'] + ['-mavx512dq'])
> +		objs += dir24_8_avx512_tmp.extract_objects('dir24_8_avx512.c')
> +		cflags += '-DCC_DIR24_8_AVX512_SUPPORT'
> +	endif
> +endif

This block looks wrong to me, especially comparing it with the equivalent
block in drivers/net/i40e. Firstly, the two if conditions are unnecessary
and can be merged. However, secondly, I think you should restructure it so
that you first check for AVX-512 already being enabled in the build, and
only if it is not do you need to see about checking compiler support and
using the static lib workaround to get just the one file compiled with
AVX-512. As Thomas suggested, a comment explaining this would also help -
again copying what is in the i40e/meson.build file would probably be a good
start.

/Bruce