Fix in v7, thanks

On 2021/5/24 18:03, Ruifeng Wang wrote:

quoted

-----Original Message-----
From: fengchengwen <redacted>
Sent: Monday, May 24, 2021 4:44 PM
To: Ruifeng Wang <redacted>; thomas@monjalon.net;
ferruh.yigit@intel.com
Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
bruce.richardson@intel.com; Honnappa Nagarahalli
[off-list ref]; jerinjacobk@gmail.com;
juraj.linkes@pantheon.tech; nd [off-list ref]
Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile method



On 2021/5/24 13:38, Ruifeng Wang wrote:

quoted

quoted

-----Original Message-----
From: fengchengwen <redacted>
Sent: Friday, May 21, 2021 2:53 PM
To: Ruifeng Wang <redacted>; thomas@monjalon.net;
ferruh.yigit@intel.com
Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
bruce.richardson@intel.com; Honnappa Nagarahalli
[off-list ref]; jerinjacobk@gmail.com;
juraj.linkes@pantheon.tech; nd [off-list ref]
Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile
method



On 2021/5/21 13:21, Ruifeng Wang wrote:

quoted

quoted

-----Original Message-----
From: fengchengwen <redacted>
Sent: Thursday, May 20, 2021 6:55 PM
To: Ruifeng Wang <redacted>; thomas@monjalon.net;
ferruh.yigit@intel.com
Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
bruce.richardson@intel.com; Honnappa Nagarahalli
[off-list ref]; jerinjacobk@gmail.com;
juraj.linkes@pantheon.tech; nd [off-list ref]
Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile
method



On 2021/5/20 16:24, Ruifeng Wang wrote:

quoted

quoted

-----Original Message-----
From: Chengwen Feng <redacted>
Sent: Wednesday, May 19, 2021 9:26 PM
To: thomas@monjalon.net; ferruh.yigit@intel.com
Cc: dev@dpdk.org; jerinj@marvell.com; Ruifeng Wang
[off-list ref]; viktorin@rehivetech.com;
bruce.richardson@intel.com; Honnappa Nagarahalli
[off-list ref]; jerinjacobk@gmail.com;
juraj.linkes@pantheon.tech; nd [off-list ref]
Subject: [PATCH v6 2/2] net/hns3: refactor SVE code compile
method

Currently, the SVE code is compiled only when -march supports SVE
(e.g. '- march=armv8.2a+sve'), there maybe some problem[1] with
this

approach.

quoted

quoted

The solution:
a. If the minimum instruction set support SVE then compiles it.
b. Else if the compiler support SVE then compiles it.
c. Otherwise don't compile it.

[1] https://mails.dpdk.org/archives/dev/2021-April/208189.html

Fixes: 8c25b02b082a ("net/hns3: fix enabling SVE Rx/Tx")
Fixes: 952ebacce4f2 ("net/hns3: support SVE Rx")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <redacted>
---
 drivers/net/hns3/hns3_rxtx.c |  2 +-
drivers/net/hns3/meson.build
|
21 ++++++++++++++++++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hns3/hns3_rxtx.c

b/drivers/net/hns3/hns3_rxtx.c index 1d7a769..4ef20c6 100644

--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c

@@ -2808,7 +2808,7 @@ hns3_get_default_vec_support(void)
 static bool
 hns3_get_sve_support(void)
 {
-#if defined(RTE_ARCH_ARM64) && defined(__ARM_FEATURE_SVE)
+#if defined(CC_SVE_SUPPORT)
 	if (rte_vect_get_max_simd_bitwidth() <

RTE_VECT_SIMD_256)

quoted

quoted

quoted

quoted

quoted

quoted

 		return false;
 	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SVE))

diff --git a/drivers/net/hns3/meson.build

b/drivers/net/hns3/meson.build index 53c7df7..5f9af9b 100644

--- a/drivers/net/hns3/meson.build
+++ b/drivers/net/hns3/meson.build

@@ -35,7 +35,26 @@ deps += ['hash']

 if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
     sources += files('hns3_rxtx_vec.c')
-    if cc.get_define('__ARM_FEATURE_SVE', args: machine_args) != ''
+
+    # compile SVE when:
+    # a. support SVE in minimum instruction set baseline
+    # b. it's not minimum instruction set, but compiler support
+    if cc.get_define('__ARM_FEATURE_SVE', args: machine_args) !=

''

quoted

quoted

quoted

quoted

quoted

quoted

+ and
cc.check_header('arm_sve.h')
+        cflags += ['-DCC_SVE_SUPPORT']

With SVE build fix patch [1], CC_SVE_ACLE_SUPPORT will be defined.
Here we can use CC_SVE_ACLE_SUPPORT and not to add a new one.

The CC_SVE_ACLE_SUPPORT was defined under default machine_args

which

quoted

quoted

support SVE, it can't deals with the situation: the default
machine_args don't support SVE but compiler support SVE.
So the CC_SVE_SUPPORT marco is necessary.

Agree that macro for SVE is also needed here. And we can also use '-

DCC_SVE_ACLE_SUPPORT' here right?

quoted

I think there is no difference between CC_SVE_ACLE_SUPPORT and

CC_SVE_SUPPORT when they are used in source code.

quoted

IMO the same macro name can be used, and it removes redundancy and

confusion.

quoted

You are right, no difference between CC_SVE_ACLE_SUPPORT and
CC_SVE_SUPPORT But the hns3 sve already support 20.11, and
CC_SVE_ACLE_SUPPORT was newly defined, there maybe some

problems when

quoted

quoted

backporting.

20.11 release has no machine enabled SVE extension.

quoted

Or we could redefine CC_SVE_ACLE_SUPPORT under default

machine_args:

quoted

quoted

    if cc.get_define('__ARM_FEATURE_SVE', args: machine_args) != ''
and
cc.check_header('arm_sve.h')
        cflags += ['-DCC_SVE_ACLE_SUPPORT']

'if dpdk_conf.get(CC_SVE_ACLE_SUPPORT)' should be fine?
Stable branch has no SVE enabled in machine_args.

But 20.11 use could use hns3 SVE path when compile with gcc10.

20.11 user will be able to use hns3 SVE path. 
Implementation 'a' should be fine for both 20.11 and 21.08.

quoted

If we reuse the CC_SVE_ACLE_SUPPORT macro, there maybe problem when
backporting:
a. In 21.08 we could depend on CC_SVE_ACLE_SUPPORT, so it will be:
    if dpdk_conf.get('CC_SVE_ACLE_SUPPORT')
        sources += files('hns3_rxtx_vec_sve.c')
    elif cc.has_argument('-march=armv8.2-a+sve') and
cc.check_header('arm_sve.h')

gcc10 user will go into this branch, and SVE path will be included.
It is identical in 'a' and 'b'.

quoted

        sve_cflags = ['-DCC_SVE_ACLE_SUPPORT']
        ...
b. But for backport to 20.11, we should use another impl:
    if cc.get_define('__ARM_FEATURE_SVE', args: machine_args) != '' and
cc.check_header('arm_sve.h')

'if' clause in implementation 'a' will have the same behavior as this one in 20.11.
 Both of the checks will be false. 
'CC_SVE_ACLE_SUPPORT' is not defined in 20.11 -> result in false.
No machine_args have sve enabled in 20.11 -> result in false.
So I think there is a chance we can use a single macro.

quoted

        cflags += ['-DCC_SVE_ACLE_SUPPORT']
        sources += files('hns3_rxtx_vec_sve.c')
    elif cc.has_argument('-march=armv8.2-a+sve') and
cc.check_header('arm_sve.h')
        sve_cflags = ['-DCC_SVE_ACLE_SUPPORT']
        ...
As you see, the above two are not unified.

So here I think use the CC_SVE_SUPPORT is appropriate.

@Ferruh  what's your opinion ?

quoted

quoted

        sources += files('hns3_rxtx_vec_sve.c')
    elif cc.has_argument('-march=armv8.2-a+sve') and
cc.check_header('arm_sve.h')
        sve_cflags = ['-DCC_SVE_ACLE_SUPPORT']

This is fine. Macro name is consistent.

quoted

        foreach flag: cflags
            # filterout -march -mcpu -mtune
            if not (flag.startswith('-march=') or
flag.startswith('-mcpu=') or
flag.startswith('-mtune='))
                sve_cflags += flag
            endif
        endforeach
but this way may introduce coupling, I think.

quoted

quoted

quoted

[1]
http://patches.dpdk.org/project/dpdk/patch/1621495007-28387-1-

git-

quoted

quoted

quoted

quoted

quoted

se
nd
-email-fengchengwen@huawei.com/

quoted

         sources += files('hns3_rxtx_vec_sve.c')
+    elif cc.has_argument('-march=armv8.2-a+sve') and
cc.check_header('arm_sve.h')
+        sve_cflags = ['-DCC_SVE_SUPPORT']
+        foreach flag: cflags
+            # filterout -march -mcpu -mtune
+            if not (flag.startswith('-march=') or
+ flag.startswith('-mcpu=') or
flag.startswith('-mtune='))
+                sve_cflags += flag
+            endif
+        endforeach
+        hns3_sve_lib = static_library('hns3_sve_lib',
+                        'hns3_rxtx_vec_sve.c',
+                        dependencies: [static_rte_ethdev],
+                        include_directories: includes,
+                        c_args: [sve_cflags, '-march=armv8.2-a+sve'])
+        objs +=
+ hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
     endif
 endif
--
2.8.1

.