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

Message ID 20190919163643.24130-3-yong.liu@intel.com
State Superseded
Headers show
Series
  • vhost packed ring performance optimization
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch warning coding style issues

Commit Message

Liu, Yong Sept. 19, 2019, 4:36 p.m.
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>

Comments

Tiwei Bie Sept. 23, 2019, 5:21 a.m. | #1
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
>
Gavin Hu (Arm Technology China) Sept. 23, 2019, 10:08 a.m. | #2
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.
Liu, Yong Sept. 24, 2019, 3:12 a.m. | #3
> -----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.

Patch

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