[dpdk-dev] [PATCH 1/2] net: add arm64 neon version of CRC compute APIs

[Jan Viktorin]

On Fri, 28 Apr 2017 10:19:20 +0000
"Sekhar, Ashwin" <Ashwin.Sekhar at 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: Viktorin at RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic