On Fri, 28 Apr 2017 10:19:20 +0000 "Sekhar, Ashwin" <ashwin.sek...@cavium.com> 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 <stdint.h> > >> +#include <assert.h> > >> + 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: vikto...@rehivetech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic
