[PATCH 1/3] efd: fix AVX2 support

[PATCH 1/3] efd: fix AVX2 support

[Thomas Monjalon]

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>
---
 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)
 	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__
 	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

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

[Bruce Richardson]

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
> 

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

[Thomas Monjalon]

18/09/2025 09:48, Bruce Richardson:
> On Thu, Sep 18, 2025 at 09:28:03AM +0200, Thomas Monjalon wrote:
> > -#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.

Correct

> > @@ -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.

Correct

I was sending you an email to explain I was at this point in this series,
but your review was too fast :)

Because compilers have the support, I suppose we could force AVX2
on this function with the impact of not being inline.
But given it was not used for years, I believe it is OK.

[PATCH v2 1/4] efd: fix AVX2 support

[Thomas Monjalon]

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 check for AVX2 is 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>
---
 lib/efd/rte_efd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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)
 	case EFD_LOOKUP_AVX2:
 		return efd_lookup_internal_avx2(group->hash_idx,
 					group->lookup_table,
-- 
2.51.0

Re: [PATCH v2 1/4] efd: fix AVX2 support

[Thomas Monjalon]

18/09/2025 11:08, Thomas Monjalon:
> 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 check for AVX2 is simply removed, and the include is added back.
[...]
> -#if defined(RTE_ARCH_X86) && defined(CC_SUPPORT_AVX2)
> +#if defined(RTE_ARCH_X86)
>  	case EFD_LOOKUP_AVX2:
>  		return efd_lookup_internal_avx2(group->hash_idx,
>  					group->lookup_table,

I've forgotten to say that there is a runtime check:

    if (RTE_EFD_VALUE_NUM_BITS > 3
            && rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2)
            && rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
        table->lookup_fn = EFD_LOOKUP_AVX2;

Re: [PATCH v2 1/4] efd: fix AVX2 support

[Thomas Monjalon]

18/09/2025 11:40, Thomas Monjalon:
> 18/09/2025 11:08, Thomas Monjalon:
> > 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 check for AVX2 is simply removed, and the include is added back.
> [...]
> > -#if defined(RTE_ARCH_X86) && defined(CC_SUPPORT_AVX2)
> > +#if defined(RTE_ARCH_X86)
> >  	case EFD_LOOKUP_AVX2:
> >  		return efd_lookup_internal_avx2(group->hash_idx,
> >  					group->lookup_table,
> 
> I've forgotten to say that there is a runtime check:
> 
>     if (RTE_EFD_VALUE_NUM_BITS > 3
>             && rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2)
>             && rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
>         table->lookup_fn = EFD_LOOKUP_AVX2;

So the end of the commit log must be changed with this explanation:

  EFD_LOOKUP_AVX2 is chosen at runtime after checking AVX2 availability,
  so the obsolete build-time check for AVX2 can be simply removed,
  and the missing include added back.