Re: [dpdk-dev] [PATCH 10/17] librte_acl: add AVX2 as new rte_acl_classify() method

[Neil Horman <nhorman@tuxdriver.com>]

On Sun, Dec 14, 2014 at 06:10:52PM +0000, Konstantin Ananyev wrote:
> Introduce new classify() method that uses AVX2 instructions.
> From my measurements:
> On HSW boards when processing >= 16 packets per call,
> AVX2 method outperforms it's SSE counterpart by 10-25%,
> (depending on the ruleset).
> At runtime, this method is selected as default one on HW that supports AVX2.
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  lib/librte_acl/Makefile       |   9 +
>  lib/librte_acl/acl.h          |   4 +
>  lib/librte_acl/acl_run.h      |   2 +-
>  lib/librte_acl/acl_run_avx2.c |  58 +++++
>  lib/librte_acl/acl_run_avx2.h | 305 ++++++++++++++++++++++++
>  lib/librte_acl/acl_run_sse.c  | 537 +-----------------------------------------
>  lib/librte_acl/acl_run_sse.h  | 533 +++++++++++++++++++++++++++++++++++++++++
>  lib/librte_acl/rte_acl.c      |   5 +-
>  lib/librte_acl/rte_acl.h      |   2 +
>  9 files changed, 917 insertions(+), 538 deletions(-)
>  create mode 100644 lib/librte_acl/acl_run_avx2.c
>  create mode 100644 lib/librte_acl/acl_run_avx2.h
>  create mode 100644 lib/librte_acl/acl_run_sse.h
> 
> diff --git a/lib/librte_acl/Makefile b/lib/librte_acl/Makefile
> index 65e566d..223ec31 100644
> --- a/lib/librte_acl/Makefile
> +++ b/lib/librte_acl/Makefile
> @@ -45,8 +45,17 @@ SRCS-$(CONFIG_RTE_LIBRTE_ACL) += acl_bld.c
>  SRCS-$(CONFIG_RTE_LIBRTE_ACL) += acl_gen.c
>  SRCS-$(CONFIG_RTE_LIBRTE_ACL) += acl_run_scalar.c
>  SRCS-$(CONFIG_RTE_LIBRTE_ACL) += acl_run_sse.c
> +SRCS-$(CONFIG_RTE_LIBRTE_ACL) += acl_run_avx2.c
>  
>  CFLAGS_acl_run_sse.o += -msse4.1
> +ifeq ($(CC), icc)
> +CFLAGS_acl_run_avx2.o += -march=core-avx2
> +else ifneq ($(shell \
> +test $(GCC_MAJOR_VERSION) -le 4 -a $(GCC_MINOR_VERSION) -le 6 && echo 1), 1)
> +CFLAGS_acl_run_avx2.o += -mavx2
> +else
> +CFLAGS_acl_run_avx2.o += -msse4.1
> +endif
>  
This seems broken.  You've unilaterally included acl_run_avx2.c in the build
list above, but only enable -mavx2 if the compiler is at least gcc 4.6.  Unless
you want to make gcc 4.6 a requirement for building, you need to also exclude
the file above from the build list.  That in turn I think allows you to remove a
bunch of the ifdeffing that you've done in some of the avx2 specific files.

Neil