[v2,02/16] vhost: unify unroll pragma parameter

  Add macro for unifying Clang/ICC/GCC unroll pragma format. Burst
functions were contained of several small loops which optimized by
compiler’s loop unrolling pragma.

Signed-off-by: Marvin Liu <yong.liu@intel.com>
  

On Fri, Sep 20, 2019 at 12:36:29AM +0800, Marvin Liu wrote:
> Add macro for unifying Clang/ICC/GCC unroll pragma format. Burst
> functions were contained of several small loops which optimized by
> compiler’s loop unrolling pragma.
> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> 
> diff --git a/lib/librte_vhost/Makefile b/lib/librte_vhost/Makefile
> index 8623e91c0..30839a001 100644
> --- a/lib/librte_vhost/Makefile
> +++ b/lib/librte_vhost/Makefile
> @@ -16,6 +16,24 @@ CFLAGS += -I vhost_user
>  CFLAGS += -fno-strict-aliasing
>  LDLIBS += -lpthread
>  
> +ifeq ($(RTE_TOOLCHAIN), gcc)
> +ifeq ($(shell test $(GCC_VERSION) -ge 83 && echo 1), 1)
> +CFLAGS += -DSUPPORT_GCC_UNROLL_PRAGMA
> +endif
> +endif
> +
> +ifeq ($(RTE_TOOLCHAIN), clang)
> +ifeq ($(shell test $(CLANG_MAJOR_VERSION)$(CLANG_MINOR_VERSION) -ge 37 && echo 1), 1)
> +CFLAGS += -DSUPPORT_CLANG_UNROLL_PRAGMA
> +endif
> +endif
> +
> +ifeq ($(RTE_TOOLCHAIN), icc)
> +ifeq ($(shell test $(ICC_MAJOR_VERSION) -ge 16 && echo 1), 1)
> +CFLAGS += -DSUPPORT_ICC_UNROLL_PRAGMA
> +endif
> +endif
> +
>  ifeq ($(CONFIG_RTE_LIBRTE_VHOST_NUMA),y)
>  LDLIBS += -lnuma
>  endif

You need to add meson support as well.

> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index 884befa85..5074226f0 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -39,6 +39,24 @@
>  
>  #define VHOST_LOG_CACHE_NR 32
>  
> +#ifdef SUPPORT_GCC_UNROLL_PRAGMA
> +#define PRAGMA_PARAM "GCC unroll 4"

The name "PRAGMA_PARAM" is too generic.

> +#endif
> +
> +#ifdef SUPPORT_CLANG_UNROLL_PRAGMA
> +#define PRAGMA_PARAM "unroll 4"
> +#endif
> +
> +#ifdef SUPPORT_ICC_UNROLL_PRAGMA
> +#define PRAGMA_PARAM "unroll (4)"
> +#endif
> +
> +#ifdef PRAGMA_PARAM
> +#define UNROLL_PRAGMA(param) _Pragma(param)
> +#else
> +#define UNROLL_PRAGMA(param) do {} while(0);
> +#endif
> +
>  /**
>   * Structure contains buffer address, length and descriptor index
>   * from vring to do scatter RX.
> -- 
> 2.17.1
>
  

Hi Marvin,

One general comment and other comments inline:
1. Meson build should also be supported as Makefile is phasing out and Meson is the future in DPDK.

/Gavin

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Marvin Liu
> Sent: Friday, September 20, 2019 12:36 AM
> To: maxime.coquelin@redhat.com; tiwei.bie@intel.com;
> zhihong.wang@intel.com
> Cc: dev@dpdk.org; Marvin Liu <yong.liu@intel.com>
> Subject: [dpdk-dev] [PATCH v2 02/16] vhost: unify unroll pragma parameter
>
> Add macro for unifying Clang/ICC/GCC unroll pragma format. Burst
> functions were contained of several small loops which optimized by
> compiler’s loop unrolling pragma.
>
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
>
> diff --git a/lib/librte_vhost/Makefile b/lib/librte_vhost/Makefile
> index 8623e91c0..30839a001 100644
> --- a/lib/librte_vhost/Makefile
> +++ b/lib/librte_vhost/Makefile
> @@ -16,6 +16,24 @@ CFLAGS += -I vhost_user
>  CFLAGS += -fno-strict-aliasing
>  LDLIBS += -lpthread
>
> +ifeq ($(RTE_TOOLCHAIN), gcc)
> +ifeq ($(shell test $(GCC_VERSION) -ge 83 && echo 1), 1)
It is better to move this toolchain version related definition to eg: mk/toolchain/icc/rte.toolchain-compat.mk.
There are a lot of similar stuff over there.
Although "CFLAGS" was added to sth under this subfolder, it still applies globally to other components.
/Gavin
> +CFLAGS += -DSUPPORT_GCC_UNROLL_PRAGMA
> +endif
> +endif
> +
> +ifeq ($(RTE_TOOLCHAIN), clang)
> +ifeq ($(shell test $(CLANG_MAJOR_VERSION)$(CLANG_MINOR_VERSION) -
> ge 37 && echo 1), 1)
> +CFLAGS += -DSUPPORT_CLANG_UNROLL_PRAGMA
Why not combine all the three "-DSUPPORT_*_UNROLL_PRAGMA" into one "-DSUPPORT_ UNROLL_PRAGMA" for simplicity?
Any differences for the support by different compilers?
/Gavin
> +endif
> +endif
> +
> +ifeq ($(RTE_TOOLCHAIN), icc)
> +ifeq ($(shell test $(ICC_MAJOR_VERSION) -ge 16 && echo 1), 1)
> +CFLAGS += -DSUPPORT_ICC_UNROLL_PRAGMA
> +endif
> +endif
> +
>  ifeq ($(CONFIG_RTE_LIBRTE_VHOST_NUMA),y)
>  LDLIBS += -lnuma
>  endif
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index 884befa85..5074226f0 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -39,6 +39,24 @@
>
>  #define VHOST_LOG_CACHE_NR 32
>
> +#ifdef SUPPORT_GCC_UNROLL_PRAGMA
> +#define PRAGMA_PARAM "GCC unroll 4"
> +#endif
> +
> +#ifdef SUPPORT_CLANG_UNROLL_PRAGMA
> +#define PRAGMA_PARAM "unroll 4"
> +#endif
> +
> +#ifdef SUPPORT_ICC_UNROLL_PRAGMA
> +#define PRAGMA_PARAM "unroll (4)"
> +#endif
> +
> +#ifdef PRAGMA_PARAM
> +#define UNROLL_PRAGMA(param) _Pragma(param)
> +#else
> +#define UNROLL_PRAGMA(param) do {} while(0);
> +#endif
> +
>  /**
>   * Structure contains buffer address, length and descriptor index
>   * from vring to do scatter RX.
> --
> 2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
  

> -----Original Message-----
> From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> Sent: Monday, September 23, 2019 6:09 PM
> To: Liu, Yong <yong.liu@intel.com>; maxime.coquelin@redhat.com; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 02/16] vhost: unify unroll pragma
> parameter
> 
> Hi Marvin,
> 
> One general comment and other comments inline:
> 1. Meson build should also be supported as Makefile is phasing out and
> Meson is the future in DPDK.
> 
> /Gavin
> 

Thanks, Gavin. Will update meson build file in next release.

> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Marvin Liu
> > Sent: Friday, September 20, 2019 12:36 AM
> > To: maxime.coquelin@redhat.com; tiwei.bie@intel.com;
> > zhihong.wang@intel.com
> > Cc: dev@dpdk.org; Marvin Liu <yong.liu@intel.com>
> > Subject: [dpdk-dev] [PATCH v2 02/16] vhost: unify unroll pragma parameter
> >
> > Add macro for unifying Clang/ICC/GCC unroll pragma format. Burst
> > functions were contained of several small loops which optimized by
> > compiler’s loop unrolling pragma.
> >
> > Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >
> > diff --git a/lib/librte_vhost/Makefile b/lib/librte_vhost/Makefile
> > index 8623e91c0..30839a001 100644
> > --- a/lib/librte_vhost/Makefile
> > +++ b/lib/librte_vhost/Makefile
> > @@ -16,6 +16,24 @@ CFLAGS += -I vhost_user
> >  CFLAGS += -fno-strict-aliasing
> >  LDLIBS += -lpthread
> >
> > +ifeq ($(RTE_TOOLCHAIN), gcc)
> > +ifeq ($(shell test $(GCC_VERSION) -ge 83 && echo 1), 1)
> It is better to move this toolchain version related definition to eg:
> mk/toolchain/icc/rte.toolchain-compat.mk.
> There are a lot of similar stuff over there.
> Although "CFLAGS" was added to sth under this subfolder, it still applies
> globally to other components.
> /Gavin
> > +CFLAGS += -DSUPPORT_GCC_UNROLL_PRAGMA
> > +endif
> > +endif
> > +
> > +ifeq ($(RTE_TOOLCHAIN), clang)
> > +ifeq ($(shell test $(CLANG_MAJOR_VERSION)$(CLANG_MINOR_VERSION) -
> > ge 37 && echo 1), 1)
> > +CFLAGS += -DSUPPORT_CLANG_UNROLL_PRAGMA
> Why not combine all the three "-DSUPPORT_*_UNROLL_PRAGMA" into one "-
> DSUPPORT_ UNROLL_PRAGMA" for simplicity?
> Any differences for the support by different compilers?
> /Gavin

Gavin,
This is due to parameter format of pragmas are different between compilers. So here created several macros for each compiler.

> > +endif
> > +endif
> > +
> > +ifeq ($(RTE_TOOLCHAIN), icc)
> > +ifeq ($(shell test $(ICC_MAJOR_VERSION) -ge 16 && echo 1), 1)
> > +CFLAGS += -DSUPPORT_ICC_UNROLL_PRAGMA
> > +endif
> > +endif
> > +
> >  ifeq ($(CONFIG_RTE_LIBRTE_VHOST_NUMA),y)
> >  LDLIBS += -lnuma
> >  endif
> > diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> > index 884befa85..5074226f0 100644
> > --- a/lib/librte_vhost/vhost.h
> > +++ b/lib/librte_vhost/vhost.h
> > @@ -39,6 +39,24 @@
> >
> >  #define VHOST_LOG_CACHE_NR 32
> >
> > +#ifdef SUPPORT_GCC_UNROLL_PRAGMA
> > +#define PRAGMA_PARAM "GCC unroll 4"
> > +#endif
> > +
> > +#ifdef SUPPORT_CLANG_UNROLL_PRAGMA
> > +#define PRAGMA_PARAM "unroll 4"
> > +#endif
> > +
> > +#ifdef SUPPORT_ICC_UNROLL_PRAGMA
> > +#define PRAGMA_PARAM "unroll (4)"
> > +#endif
> > +
> > +#ifdef PRAGMA_PARAM
> > +#define UNROLL_PRAGMA(param) _Pragma(param)
> > +#else
> > +#define UNROLL_PRAGMA(param) do {} while(0);
> > +#endif
> > +
> >  /**
> >   * Structure contains buffer address, length and descriptor index
> >   * from vring to do scatter RX.
> > --
> > 2.17.1
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.