Re: [PATCH 1/3] efd: fix AVX2 support

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

On Thu, Sep 18, 2025 at 09:28:03AM +0200, Thomas Monjalon wrote:
> When switching to Meson build, the compilation check on CC_SUPPORT_AVX2
> became obsolete, thus the case EFD_LOOKUP_AVX2 became dead.
> The function efd_lookup_internal_avx2() was never called,
> and its header include rte_efd_x86.h has been removed later.
> 
> AVX2 is assumed to be always supported on x86 with supported compilers,
> so the checks for AVX2 are simply removed, and the include is added back.
> 
> Fixes: 5b9656b157d3 ("lib: build with meson")
> Fixes: 30a1de105a5f ("lib: remove unneeded header includes")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Some comments inline below. Not sure all these removals are entirely
correct - though it may be that the original code has issues itself, rather
than just this patch.

/Bruce

> ---
>  lib/efd/rte_efd.c     |  3 ++-
>  lib/efd/rte_efd_x86.h | 10 ----------
>  2 files changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/efd/rte_efd.c b/lib/efd/rte_efd.c
> index b0e44e5c51..ebf1e0655f 100644
> --- a/lib/efd/rte_efd.c
> +++ b/lib/efd/rte_efd.c
> @@ -26,6 +26,7 @@
>  
>  #include "rte_efd.h"
>  #if defined(RTE_ARCH_X86)
> +#include "rte_efd_x86.h"
>  #elif defined(RTE_ARCH_ARM64)
>  #include "rte_efd_arm64.h"
>  #endif
> @@ -1279,7 +1280,7 @@ efd_lookup_internal(const struct efd_online_group_entry * const group,
>  
>  	switch (lookup_fn) {
>  
> -#if defined(RTE_ARCH_X86) && defined(CC_SUPPORT_AVX2)
> +#if defined(RTE_ARCH_X86)

Ok to remove this, because indeed all supported compilers have AVX2.

However, given that the efd meson.build file doesn't check for compiler
support and optionally build some extra files with the AVX2 flags, I wonder
if this define should actually be changed to an __AVX2__ one, to detect if
the build has AVX2 support rather than just the compiler.

>  	case EFD_LOOKUP_AVX2:
>  		return efd_lookup_internal_avx2(group->hash_idx,
>  					group->lookup_table,
> diff --git a/lib/efd/rte_efd_x86.h b/lib/efd/rte_efd_x86.h
> index e2f9dcca88..f7776db8a5 100644
> --- a/lib/efd/rte_efd_x86.h
> +++ b/lib/efd/rte_efd_x86.h
> @@ -19,7 +19,6 @@ efd_lookup_internal_avx2(const efd_hashfunc_t *group_hash_idx,
>  		const efd_lookuptbl_t *group_lookup_table,
>  		const uint32_t hash_val_a, const uint32_t hash_val_b)
>  {
> -#ifdef __AVX2__

This may not be safe to remove though, because AVX2 support may not
actually be present in the build. For example, when doing a default build
with -march=corei7, __AVX2__ will not be defined, because the target CPU
doesn't support it, even though the compiler does.

>  	efd_value_t value = 0;
>  	uint32_t i = 0;
>  	__m256i vhash_val_a = _mm256_set1_epi32(hash_val_a);
> @@ -45,13 +44,4 @@ efd_lookup_internal_avx2(const efd_hashfunc_t *group_hash_idx,
>  	}
>  
>  	return value;
> -#else
> -	RTE_SET_USED(group_hash_idx);
> -	RTE_SET_USED(group_lookup_table);
> -	RTE_SET_USED(hash_val_a);
> -	RTE_SET_USED(hash_val_b);
> -	/* Return dummy value, only to avoid compilation breakage */
> -	return 0;
> -#endif
> -
>  }
> -- 
> 2.51.0
>