From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wes1-so1.wedos.net (wes1-so1-c.wedos.net [46.28.106.44]) by dpdk.org (Postfix) with ESMTP id AD1FC58D1 for ; Wed, 3 May 2017 18:59:21 +0200 (CEST) Received: from pcviktorin.fit.vutbr.cz (dhcpz185.fit.vutbr.cz [147.229.14.185]) by wes1-so1.wedos.net (Postfix) with ESMTPSA id 3wJ4Bd12dYz25B; Wed, 3 May 2017 18:59:21 +0200 (CEST) Date: Wed, 3 May 2017 18:58:00 +0200 From: Jan Viktorin To: "Sekhar, Ashwin" Cc: "thomas@monjalon.net" , "jasvinder.singh@intel.com" , "Jacob, Jerin" , "jianbo.liu@linaro.org" , "dev@dpdk.org" Message-ID: <20170503185800.1f44eb2f.viktorin@rehivetech.com> In-Reply-To: References: <20170427141021.18767-1-ashwin.sekhar@caviumnetworks.com> <20170428115544.52d3388d@jvn> Organization: RehiveTech MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/2] net: add arm64 neon version of CRC compute APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 May 2017 16:59:22 -0000 On Fri, 28 Apr 2017 10:19:20 +0000 "Sekhar, Ashwin" wrote: > Hi Jan, > Thanks for the comments. Please see my responses inline. > > On Friday 28 April 2017 03:25 PM, Jan Viktorin wrote: > > Hello Ashwin Sekhar, > > > > some comments below... > > [...] > >> > >> #include > >> +#include > >> + I'd prefer RTE_ASSERT (rte_debug.h) instead of this one. > >> #include "generic/rte_vect.h" > >> #include "arm_neon.h" > >> > >> +#ifdef GCC_VERSION > >> +#undef GCC_VERSION > >> +#endif > > > > Why are you doing this? What is wrong with GCC_VERSION? > > > This is just to avoid multiple definitions of GCC_VERSION. Not required > really. Can be removed. > > >> + > >> +#define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 \ > >> + + __GNUC_PATCHLEVEL__) > >> + > > > > If you have any specific requirements for testing GCC version then it > > should be done in a more elegant way. However, I do not understand your > > intention. > > > GCC version is checked so as to define wrappers for some neon intrinsics > which are not available in GCC versions < 7. > > Similar checks of GCC_VERSION done in ./lib/librte_table/rte_lru.h. > Followed the same template here. > Also, this is the suggested approach by GCC. Please see below link. > https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html This is OK, I understand. > > Please advise on more elegant ways of gcc version detection. I don't say that it is wrong. Just, it is quite a low-level definition that might be solved once for all to avoid any further GCC_VERSION definitions. At least, I would go this way: #ifdef __GNUC__ #define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) #else #define GCC_VERSION 0 #endif To be better, this should be defined in some more general file (rte_common.h, rte_compiler.h)? I don't have any strong opinion about this. The rte_lru.h can be refactorized in the same way. > >> #ifdef __cplusplus > >> extern "C" { > >> #endif > >> @@ -78,6 +87,42 @@ vqtbl1q_u8(uint8x16_t a, uint8x16_t b) > >> } > >> #endif > >> > >> +#if (GCC_VERSION < 70000) > > [...] > > > >> # CPU_LDFLAGS = > >> # CPU_ASFLAGS = > >> > >> -MACHINE_CFLAGS += -march=armv8-a+crc > >> +MACHINE_CFLAGS += -march=armv8-a+crc+crypto > >> diff --git a/mk/machine/thunderx/rte.vars.mk b/mk/machine/thunderx/rte.vars.mk > >> index ad5a379..6784105 100644 > >> --- a/mk/machine/thunderx/rte.vars.mk > >> +++ b/mk/machine/thunderx/rte.vars.mk > >> @@ -55,4 +55,4 @@ > >> # CPU_LDFLAGS = > >> # CPU_ASFLAGS = > >> > >> -MACHINE_CFLAGS += -march=armv8-a+crc -mcpu=thunderx > >> +MACHINE_CFLAGS += -march=armv8-a+crc+crypto -mcpu=thunderx > >> diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk > >> index e634abc..6bbd742 100644 > >> --- a/mk/rte.cpuflags.mk > >> +++ b/mk/rte.cpuflags.mk > >> @@ -119,6 +119,9 @@ ifneq ($(filter $(AUTO_CPUFLAGS),__ARM_FEATURE_CRC32),) > >> CPUFLAGS += CRC32 > >> endif > >> > >> +ifneq ($(filter $(AUTO_CPUFLAGS),__ARM_FEATURE_CRYPTO),) > >> +CPUFLAGS += PMULL > >> +endif > >> > >> MACHINE_CFLAGS += $(addprefix -DRTE_MACHINE_CPUFLAG_,$(CPUFLAGS)) > >> > >> diff --git a/mk/toolchain/gcc/rte.toolchain-compat.mk b/mk/toolchain/gcc/rte.toolchain-compat.mk > >> index 280dde2..01ac7e2 100644 > >> --- a/mk/toolchain/gcc/rte.toolchain-compat.mk > >> +++ b/mk/toolchain/gcc/rte.toolchain-compat.mk > >> @@ -60,6 +60,7 @@ else > >> # > >> ifeq ($(shell test $(GCC_VERSION) -le 49 && echo 1), 1) > >> MACHINE_CFLAGS := $(patsubst -march=armv8-a+crc,-march=armv8-a+crc -D__ARM_FEATURE_CRC32=1,$(MACHINE_CFLAGS)) > > > > The line above is to be dropped, isn't it? > > > No. It is not to be dropped. For targets like xgene1, crypto is not > defined. Above line is required for the substitution to happen in such > targets. Yes, I understand... > >> + MACHINE_CFLAGS := $(patsubst -march=armv8-a+crc+crypto,-march=armv8-a+crc+crypto -D__ARM_FEATURE_CRC32=1,$(MACHINE_CFLAGS)) > > > > Please, split the "feature-detection" changes into a separate commit and > > explain it. In the code, you test for GCC 7. Here you are ok with GCC > > 4.9. It's likely to be correct but it is not clear. > Sure. Will split the feature detection changes to separate commit. > > > > Also, please explain why is the "crypto" feature required. > crypto feature is required for using the vmull_p64 intrinsic. More > specifically the PMULL instruction. > Will add this as part of the commit message. OK. Jan > > > > Regards > > Jan > > > >> endif > >> ifeq ($(shell test $(GCC_VERSION) -le 47 && echo 1), 1) > >> MACHINE_CFLAGS := $(patsubst -march=core-avx-i,-march=corei7-avx,$(MACHINE_CFLAGS)) > > > Thanks and Regards, > Ashwin > -- Jan Viktorin E-mail: Viktorin@RehiveTech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic