From: "Sekhar, Ashwin" <Ashwin.Sekhar@cavium.com>
To: Jan Viktorin <viktorin@rehivetech.com>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
"jasvinder.singh@intel.com" <jasvinder.singh@intel.com>,
"Jacob, Jerin" <Jerin.JacobKollanukkaran@cavium.com>,
"jianbo.liu@linaro.org" <jianbo.liu@linaro.org>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 1/2] net: add arm64 neon version of CRC compute APIs
Date: Fri, 28 Apr 2017 10:19:20 +0000 [thread overview]
Message-ID: <BY2PR07MB242149B08750D0CA13A57A4F92130@BY2PR07MB2421.namprd07.prod.outlook.com> (raw)
In-Reply-To: <20170428115544.52d3388d@jvn>
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...
>
> On Thu, 27 Apr 2017 07:10:20 -0700
> Ashwin Sekhar T K <ashwin.sekhar@caviumnetworks.com> wrote:
>
>> * Added CRC compute APIs for arm64 utilizing the pmull capability
>> * Added new file net_crc_neon.h to hold the arm64 pmull CRC
>> implementation
>> * Added crypto capability in compilation of generic armv8 and
>> thunderx targets
>> * pmull CRC version is used only after checking the pmull capability
>> at runtime
>> * Verified the changes with crc_autotest unit test case
>>
>> Signed-off-by: Ashwin Sekhar T K <ashwin.sekhar@caviumnetworks.com>
>> ---
>> MAINTAINERS | 1 +
>> lib/librte_eal/common/include/arch/arm/rte_vect.h | 45 +++
>> lib/librte_net/net_crc_neon.h | 357 ++++++++++++++++++++++
>> lib/librte_net/rte_net_crc.c | 32 +-
>> lib/librte_net/rte_net_crc.h | 2 +
>> mk/machine/armv8a/rte.vars.mk | 2 +-
>> mk/machine/thunderx/rte.vars.mk | 2 +-
>> mk/rte.cpuflags.mk | 3 +
>> mk/toolchain/gcc/rte.toolchain-compat.mk | 1 +
>> 9 files changed, 438 insertions(+), 7 deletions(-)
>> create mode 100644 lib/librte_net/net_crc_neon.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 576d60a..283743e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -149,6 +149,7 @@ F: lib/librte_lpm/rte_lpm_neon.h
>> F: lib/librte_hash/rte*_arm64.h
>> F: lib/librte_efd/rte*_arm64.h
>> F: lib/librte_table/rte*_arm64.h
>> +F: lib/librte_net/net_crc_neon.h
>> F: drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c
>> F: drivers/net/i40e/i40e_rxtx_vec_neon.c
>> F: drivers/net/virtio/virtio_rxtx_simple_neon.c
>> diff --git a/lib/librte_eal/common/include/arch/arm/rte_vect.h b/lib/librte_eal/common/include/arch/arm/rte_vect.h
>> index 4107c99..9a3dfdf 100644
>> --- a/lib/librte_eal/common/include/arch/arm/rte_vect.h
>> +++ b/lib/librte_eal/common/include/arch/arm/rte_vect.h
>> @@ -34,9 +34,18 @@
>> #define _RTE_VECT_ARM_H_
>>
>> #include <stdint.h>
>> +#include <assert.h>
>> +
>> #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
Please advise on more elegant ways of gcc version detection.
>> #ifdef __cplusplus
>> extern "C" {
>> #endif
>> @@ -78,6 +87,42 @@ vqtbl1q_u8(uint8x16_t a, uint8x16_t b)
>> }
>> #endif
>>
>> +#if (GCC_VERSION < 70000)
>
> Is this code is gcc-specific? In such case there should be check for
> GCC compiler. We can also build e.g. by clang.
>
Yes, the code is GCC specific. Currently there are only GCC targets for
arm and arm64. So no checks are done for other types of compilers.
>> +/*
>> + * NEON intrinsic vreinterpretq_u64_p128() is not supported
>> + * in GCC versions < 7
>> + */
>
> I'd be positive about those comments, like:
>
> NEON intrinsic vreinterpretq_u64_p128() is supported since GCC 7.
>
Thanks. Will make the comments positive.
>> +static inline uint64x2_t
>> +vreinterpretq_u64_p128(poly128_t x)
>> +{
>> + return (uint64x2_t)x;
>> +}
>> +
>> +/*
>> + * NEON intrinsic vreinterpretq_p64_u64() is not supported
>> + * in GCC versions < 7
>> + */
>> +static inline poly64x2_t
>> +vreinterpretq_p64_u64(uint64x2_t x)
>> +{
>> + return (poly64x2_t)x;
>> +}
>> +
>> +/*
>> + * NEON intrinsic vgetq_lane_p64() is not supported
>> + * in GCC versions < 7
>> + */
>> +static inline poly64_t
>> +vgetq_lane_p64(poly64x2_t x, const int lane)
>> +{
>> + assert(lane >= 0 && lane <= 1);
>> +
>> + poly64_t *p = (poly64_t *)&x;
>> +
>> + return p[lane];
>> +}
>> +#endif
>> +
>> #ifdef __cplusplus
>> }
>> #endif
>> diff --git a/lib/librte_net/net_crc_neon.h b/lib/librte_net/net_crc_neon.h
>
> [...]
>
>> # 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.
>> + 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.
>
> 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
next prev parent reply other threads:[~2017-04-28 10:19 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-27 14:10 Ashwin Sekhar T K
2017-04-27 14:10 ` [dpdk-dev] [PATCH 2/2] test: add tests for arm64 CRC neon versions Ashwin Sekhar T K
2017-04-28 9:34 ` [dpdk-dev] [PATCH v2 1/2] net: add arm64 neon version of CRC compute APIs Ashwin Sekhar T K
2017-04-28 9:34 ` [dpdk-dev] [PATCH v2 2/2] test: add tests for arm64 CRC neon versions Ashwin Sekhar T K
2017-04-28 9:55 ` [dpdk-dev] [PATCH 1/2] net: add arm64 neon version of CRC compute APIs Jan Viktorin
2017-04-28 10:19 ` Sekhar, Ashwin [this message]
2017-05-03 16:58 ` Jan Viktorin
-- strict thread matches above, loose matches on Subject: below --
2017-04-27 14:06 Ashwin Sekhar T K
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BY2PR07MB242149B08750D0CA13A57A4F92130@BY2PR07MB2421.namprd07.prod.outlook.com \
--to=ashwin.sekhar@cavium.com \
--cc=Jerin.JacobKollanukkaran@cavium.com \
--cc=dev@dpdk.org \
--cc=jasvinder.singh@intel.com \
--cc=jianbo.liu@linaro.org \
--cc=thomas@monjalon.net \
--cc=viktorin@rehivetech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).