diff mbox series

[v6,2/2] net/hns3: refactor SVE code compile method

Message ID 1621430731-27753-3-git-send-email-fengchengwen@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series bugfix for Kunpeng SVE compile |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-abi-testing success Testing PASS
ci/iol-testing fail Testing issues
ci/github-robot success github build: passed
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance fail Performance Testing issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-mellanox-Functional fail Functional Testing issues

Commit Message

Chengwen Feng May 19, 2021, 1:25 p.m. UTC 
  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.

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 <fengchengwen@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c |  2 +-
 drivers/net/hns3/meson.build | 21 ++++++++++++++++++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit May 19, 2021, 3:02 p.m. UTC | #1 
On 5/19/2021 2:25 PM, Chengwen Feng wrote:
> 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.
> 
> 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 <fengchengwen@huawei.com>

The patch passes the CI build [1], but in that test sve file is not compiled at
all because of missing header file [2].

I wonder if the warning caused by conflicting switch [3] is still valid, we need
a platform that sve file is compiled to verify this. Do you have this
environment, that sets '-mcpu=armv8.1-a'.


Btw, CI reports unit test failure, I don't think it is related with this patch
but can you please double check?



[1]
https://lab.dpdk.org/results/dashboard/patchsets/17135/

[2]
Check usable header "arm_sve.h" : NO

[3]
error: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’ switch [-Werror]

> ---
>  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)
>  		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) != '' and cc.check_header('arm_sve.h')
> +        cflags += ['-DCC_SVE_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_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
>
Chengwen Feng May 20, 2021, 1:11 a.m. UTC | #2 
On 2021/5/19 23:02, Ferruh Yigit wrote:
> On 5/19/2021 2:25 PM, Chengwen Feng wrote:
>> 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.
>>
>> 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 <fengchengwen@huawei.com>
> 
> The patch passes the CI build [1], but in that test sve file is not compiled at
> all because of missing header file [2].
> 

Yes, it was designed as it.
In hns3 meson.build we call cc.check_header('arm_sve.h'), and gcc9 don't have this headerfile.

> I wonder if the warning caused by conflicting switch [3] is still valid, we need
> a platform that sve file is compiled to verify this. Do you have this
> environment, that sets '-mcpu=armv8.1-a'.
> 

It already fix by filterout '-march' '-mcpu' '-mtune' in hns3 meson.build of this patch
If don't add the above filtout logic:
  a) Test result with gcc8.3 and crossfile thunder2:
    cc1: warning: switch ‘-mcpu=thunderx2t99’ conflicts with ‘-march=armv8.2-a+sve’ switch
  b) Test result with gcc9.2 and crossfile thunder2:
    cc1: warning: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’ switch

Note: the gcc8.3/9.2 version detail:
  ./aarch64-linux-gnu-gcc --version
  aarch64-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 8.3-2019.03 (arm-rel-8.36)) 8.3.0
  ./aarch64-none-linux-gnu-gcc --version
  aarch64-none-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 9.2-2019.12 (arm-9.10)) 9.2.1 20191025

> 
> Btw, CI reports unit test failure, I don't think it is related with this patch
> but can you please double check?
> 

Agree, it is atomic_autotest and malloc_autotest failed, it hasn't run any hns3 driver's code.

> 
> 
> [1]
> https://lab.dpdk.org/results/dashboard/patchsets/17135/
> 
> [2]
> Check usable header "arm_sve.h" : NO
> 
> [3]
> error: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’ switch [-Werror]
> 
>> ---
>>  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)
>>  		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) != '' and cc.check_header('arm_sve.h')
>> +        cflags += ['-DCC_SVE_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_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
>>
> 
> 
> .
>
Ferruh Yigit May 20, 2021, 7:57 a.m. UTC | #3 
On 5/20/2021 2:11 AM, fengchengwen wrote:
> 
> 
> On 2021/5/19 23:02, Ferruh Yigit wrote:
>> On 5/19/2021 2:25 PM, Chengwen Feng wrote:
>>> 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.
>>>
>>> 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 <fengchengwen@huawei.com>
>>
>> The patch passes the CI build [1], but in that test sve file is not compiled at
>> all because of missing header file [2].
>>
> 
> Yes, it was designed as it.
> In hns3 meson.build we call cc.check_header('arm_sve.h'), and gcc9 don't have this headerfile.
> 
>> I wonder if the warning caused by conflicting switch [3] is still valid, we need
>> a platform that sve file is compiled to verify this. Do you have this
>> environment, that sets '-mcpu=armv8.1-a'.
>>
> 
> It already fix by filterout '-march' '-mcpu' '-mtune' in hns3 meson.build of this patch
> If don't add the above filtout logic:
>   a) Test result with gcc8.3 and crossfile thunder2:
>     cc1: warning: switch ‘-mcpu=thunderx2t99’ conflicts with ‘-march=armv8.2-a+sve’ switch
>   b) Test result with gcc9.2 and crossfile thunder2:
>     cc1: warning: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’ switch
> 
> Note: the gcc8.3/9.2 version detail:
>   ./aarch64-linux-gnu-gcc --version
>   aarch64-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 8.3-2019.03 (arm-rel-8.36)) 8.3.0
>   ./aarch64-none-linux-gnu-gcc --version
>   aarch64-none-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 9.2-2019.12 (arm-9.10)) 9.2.1 20191025
> 

Hi Chengwen,

-rc4 is already out and release is planned for tomorrow, so it is late to get
this patch now, let's proceed with it in next release.

>>
>> Btw, CI reports unit test failure, I don't think it is related with this patch
>> but can you please double check?
>>
> 
> Agree, it is atomic_autotest and malloc_autotest failed, it hasn't run any hns3 driver's code.
> 
>>
>>
>> [1]
>> https://lab.dpdk.org/results/dashboard/patchsets/17135/
>>
>> [2]
>> Check usable header "arm_sve.h" : NO
>>
>> [3]
>> error: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’ switch [-Werror]
>>
>>> ---
>>>  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)
>>>  		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) != '' and cc.check_header('arm_sve.h')
>>> +        cflags += ['-DCC_SVE_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_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
>>>
>>
>>
>> .
>>
>
Ruifeng Wang May 20, 2021, 8:24 a.m. UTC | #4 
> -----Original Message-----
> From: Chengwen Feng <fengchengwen@huawei.com>
> 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
> <Ruifeng.Wang@arm.com>; viktorin@rehivetech.com;
> bruce.richardson@intel.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
> juraj.linkes@pantheon.tech; nd <nd@arm.com>
> 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.
> 
> 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 <fengchengwen@huawei.com>
> ---
>  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)
>  		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) != '' 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.

[1] http://patches.dpdk.org/project/dpdk/patch/1621495007-28387-1-git-send-email-fengchengwen@huawei.com/

>          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
Chengwen Feng May 20, 2021, 10:55 a.m. UTC | #5 
On 2021/5/20 16:24, Ruifeng Wang wrote:
>> -----Original Message-----
>> From: Chengwen Feng <fengchengwen@huawei.com>
>> 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
>> <Ruifeng.Wang@arm.com>; viktorin@rehivetech.com;
>> bruce.richardson@intel.com; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>> 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.
>>
>> 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 <fengchengwen@huawei.com>
>> ---
>>  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)
>>  		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) != '' 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 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.

> [1] http://patches.dpdk.org/project/dpdk/patch/1621495007-28387-1-git-send-email-fengchengwen@huawei.com/
> 
>>          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
> 
> 
> .
>
Ruifeng Wang May 21, 2021, 5:21 a.m. UTC | #6 
> -----Original Message-----
> From: fengchengwen <fengchengwen@huawei.com>
> Sent: Thursday, May 20, 2021 6:55 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
> ferruh.yigit@intel.com
> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
> bruce.richardson@intel.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
> juraj.linkes@pantheon.tech; nd <nd@arm.com>
> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile method
> 
> 
> 
> On 2021/5/20 16:24, Ruifeng Wang wrote:
> >> -----Original Message-----
> >> From: Chengwen Feng <fengchengwen@huawei.com>
> >> 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
> >> <Ruifeng.Wang@arm.com>; viktorin@rehivetech.com;
> >> bruce.richardson@intel.com; Honnappa Nagarahalli
> >> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
> >> juraj.linkes@pantheon.tech; nd <nd@arm.com>
> >> 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.
> >>
> >> 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 <fengchengwen@huawei.com>
> >> ---
> >>  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)
> >>  		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) != ''
> >> + 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
> 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?
I think there is no difference between CC_SVE_ACLE_SUPPORT and CC_SVE_SUPPORT when they are used in source code.
IMO the same macro name can be used, and it removes redundancy and confusion. 

> 
> > [1]
> > http://patches.dpdk.org/project/dpdk/patch/1621495007-28387-1-git-send
> > -email-fengchengwen@huawei.com/
> >
> >>          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
> >
> >
> > .
> >
Chengwen Feng May 21, 2021, 6:53 a.m. UTC | #7 
On 2021/5/21 13:21, Ruifeng Wang wrote:
>> -----Original Message-----
>> From: fengchengwen <fengchengwen@huawei.com>
>> Sent: Thursday, May 20, 2021 6:55 PM
>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
>> ferruh.yigit@intel.com
>> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
>> bruce.richardson@intel.com; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile method
>>
>>
>>
>> On 2021/5/20 16:24, Ruifeng Wang wrote:
>>>> -----Original Message-----
>>>> From: Chengwen Feng <fengchengwen@huawei.com>
>>>> 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
>>>> <Ruifeng.Wang@arm.com>; viktorin@rehivetech.com;
>>>> bruce.richardson@intel.com; Honnappa Nagarahalli
>>>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
>>>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>>>> 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.
>>>>
>>>> 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 <fengchengwen@huawei.com>
>>>> ---
>>>>  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)
>>>>  		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) != ''
>>>> + 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
>> 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?
> I think there is no difference between CC_SVE_ACLE_SUPPORT and CC_SVE_SUPPORT when they are used in source code.
> IMO the same macro name can be used, and it removes redundancy and confusion. 
> 

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 backporting.

Or we could redefine CC_SVE_ACLE_SUPPORT under default machine_args:
    if cc.get_define('__ARM_FEATURE_SVE', args: machine_args) != '' and cc.check_header('arm_sve.h')
        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']
        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.

>>
>>> [1]
>>> http://patches.dpdk.org/project/dpdk/patch/1621495007-28387-1-git-send
>>> -email-fengchengwen@huawei.com/
>>>
>>>>          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
>>>
>>>
>>> .
>>>
>
Ruifeng Wang May 24, 2021, 5:38 a.m. UTC | #8 
> -----Original Message-----
> From: fengchengwen <fengchengwen@huawei.com>
> Sent: Friday, May 21, 2021 2:53 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
> ferruh.yigit@intel.com
> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
> bruce.richardson@intel.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
> juraj.linkes@pantheon.tech; nd <nd@arm.com>
> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile method
> 
> 
> 
> On 2021/5/21 13:21, Ruifeng Wang wrote:
> >> -----Original Message-----
> >> From: fengchengwen <fengchengwen@huawei.com>
> >> Sent: Thursday, May 20, 2021 6:55 PM
> >> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
> >> ferruh.yigit@intel.com
> >> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
> >> bruce.richardson@intel.com; Honnappa Nagarahalli
> >> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
> >> juraj.linkes@pantheon.tech; nd <nd@arm.com>
> >> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile
> >> method
> >>
> >>
> >>
> >> On 2021/5/20 16:24, Ruifeng Wang wrote:
> >>>> -----Original Message-----
> >>>> From: Chengwen Feng <fengchengwen@huawei.com>
> >>>> 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
> >>>> <Ruifeng.Wang@arm.com>; viktorin@rehivetech.com;
> >>>> bruce.richardson@intel.com; Honnappa Nagarahalli
> >>>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
> >>>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
> >>>> 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.
> >>>>
> >>>> 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 <fengchengwen@huawei.com>
> >>>> ---
> >>>>  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)
> >>>>  		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) != ''
> >>>> + 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
> >> 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?
> > I think there is no difference between CC_SVE_ACLE_SUPPORT and
> CC_SVE_SUPPORT when they are used in source code.
> > IMO the same macro name can be used, and it removes redundancy and
> confusion.
> >
> 
> 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 backporting.
20.11 release has no machine enabled SVE extension. 

> 
> Or we could redefine CC_SVE_ACLE_SUPPORT under default machine_args:
>     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.

>         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.

>         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.
> 
> >>
> >>> [1]
> >>> http://patches.dpdk.org/project/dpdk/patch/1621495007-28387-1-git-se
> >>> nd
> >>> -email-fengchengwen@huawei.com/
> >>>
> >>>>          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
> >>>
> >>>
> >>> .
> >>>
> >
Chengwen Feng May 24, 2021, 8:43 a.m. UTC | #9 
On 2021/5/24 13:38, Ruifeng Wang wrote:
>> -----Original Message-----
>> From: fengchengwen <fengchengwen@huawei.com>
>> Sent: Friday, May 21, 2021 2:53 PM
>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
>> ferruh.yigit@intel.com
>> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
>> bruce.richardson@intel.com; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile method
>>
>>
>>
>> On 2021/5/21 13:21, Ruifeng Wang wrote:
>>>> -----Original Message-----
>>>> From: fengchengwen <fengchengwen@huawei.com>
>>>> Sent: Thursday, May 20, 2021 6:55 PM
>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
>>>> ferruh.yigit@intel.com
>>>> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
>>>> bruce.richardson@intel.com; Honnappa Nagarahalli
>>>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
>>>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>>>> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile
>>>> method
>>>>
>>>>
>>>>
>>>> On 2021/5/20 16:24, Ruifeng Wang wrote:
>>>>>> -----Original Message-----
>>>>>> From: Chengwen Feng <fengchengwen@huawei.com>
>>>>>> 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
>>>>>> <Ruifeng.Wang@arm.com>; viktorin@rehivetech.com;
>>>>>> bruce.richardson@intel.com; Honnappa Nagarahalli
>>>>>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
>>>>>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>>>>>> 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.
>>>>>>
>>>>>> 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 <fengchengwen@huawei.com>
>>>>>> ---
>>>>>>  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)
>>>>>>  		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) != ''
>>>>>> + 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
>>>> 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?
>>> I think there is no difference between CC_SVE_ACLE_SUPPORT and
>> CC_SVE_SUPPORT when they are used in source code.
>>> IMO the same macro name can be used, and it removes redundancy and
>> confusion.
>>>
>>
>> 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 backporting.
> 20.11 release has no machine enabled SVE extension. 
> 
>>
>> Or we could redefine CC_SVE_ACLE_SUPPORT under default machine_args:
>>     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.

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')
        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')
        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 ?

>>         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.
> 
>>         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.
>>
>>>>
>>>>> [1]
>>>>> http://patches.dpdk.org/project/dpdk/patch/1621495007-28387-1-git-se
>>>>> nd
>>>>> -email-fengchengwen@huawei.com/
>>>>>
>>>>>>          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
>>>>>
>>>>>
>>>>> .
>>>>>
>>>
>
Ruifeng Wang May 24, 2021, 10:03 a.m. UTC | #10 
> -----Original Message-----
> From: fengchengwen <fengchengwen@huawei.com>
> Sent: Monday, May 24, 2021 4:44 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
> ferruh.yigit@intel.com
> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
> bruce.richardson@intel.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
> juraj.linkes@pantheon.tech; nd <nd@arm.com>
> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile method
> 
> 
> 
> On 2021/5/24 13:38, Ruifeng Wang wrote:
> >> -----Original Message-----
> >> From: fengchengwen <fengchengwen@huawei.com>
> >> Sent: Friday, May 21, 2021 2:53 PM
> >> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
> >> ferruh.yigit@intel.com
> >> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
> >> bruce.richardson@intel.com; Honnappa Nagarahalli
> >> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
> >> juraj.linkes@pantheon.tech; nd <nd@arm.com>
> >> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile
> >> method
> >>
> >>
> >>
> >> On 2021/5/21 13:21, Ruifeng Wang wrote:
> >>>> -----Original Message-----
> >>>> From: fengchengwen <fengchengwen@huawei.com>
> >>>> Sent: Thursday, May 20, 2021 6:55 PM
> >>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
> >>>> ferruh.yigit@intel.com
> >>>> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
> >>>> bruce.richardson@intel.com; Honnappa Nagarahalli
> >>>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
> >>>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
> >>>> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile
> >>>> method
> >>>>
> >>>>
> >>>>
> >>>> On 2021/5/20 16:24, Ruifeng Wang wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Chengwen Feng <fengchengwen@huawei.com>
> >>>>>> 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
> >>>>>> <Ruifeng.Wang@arm.com>; viktorin@rehivetech.com;
> >>>>>> bruce.richardson@intel.com; Honnappa Nagarahalli
> >>>>>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
> >>>>>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
> >>>>>> 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.
> >>>>>>
> >>>>>> 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 <fengchengwen@huawei.com>
> >>>>>> ---
> >>>>>>  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)
> >>>>>>  		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) !=
> ''
> >>>>>> + 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
> >>>> 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?
> >>> I think there is no difference between CC_SVE_ACLE_SUPPORT and
> >> CC_SVE_SUPPORT when they are used in source code.
> >>> IMO the same macro name can be used, and it removes redundancy and
> >> confusion.
> >>>
> >>
> >> 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
> >> backporting.
> > 20.11 release has no machine enabled SVE extension.
> >
> >>
> >> Or we could redefine CC_SVE_ACLE_SUPPORT under default
> machine_args:
> >>     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.

> 
> 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'.

>         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.

>         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 ?
> 
> >>         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.
> >
> >>         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.
> >>
> >>>>
> >>>>> [1]
> >>>>> http://patches.dpdk.org/project/dpdk/patch/1621495007-28387-1-
> git-
> >>>>> se
> >>>>> nd
> >>>>> -email-fengchengwen@huawei.com/
> >>>>>
> >>>>>>          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
> >>>>>
> >>>>>
> >>>>> .
> >>>>>
> >>>
> >
Chengwen Feng May 24, 2021, 1:15 p.m. UTC | #11 
Fix in v7, thanks

On 2021/5/24 18:03, Ruifeng Wang wrote:
>> -----Original Message-----
>> From: fengchengwen <fengchengwen@huawei.com>
>> Sent: Monday, May 24, 2021 4:44 PM
>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
>> ferruh.yigit@intel.com
>> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
>> bruce.richardson@intel.com; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile method
>>
>>
>>
>> On 2021/5/24 13:38, Ruifeng Wang wrote:
>>>> -----Original Message-----
>>>> From: fengchengwen <fengchengwen@huawei.com>
>>>> Sent: Friday, May 21, 2021 2:53 PM
>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
>>>> ferruh.yigit@intel.com
>>>> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
>>>> bruce.richardson@intel.com; Honnappa Nagarahalli
>>>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
>>>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>>>> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile
>>>> method
>>>>
>>>>
>>>>
>>>> On 2021/5/21 13:21, Ruifeng Wang wrote:
>>>>>> -----Original Message-----
>>>>>> From: fengchengwen <fengchengwen@huawei.com>
>>>>>> Sent: Thursday, May 20, 2021 6:55 PM
>>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
>>>>>> ferruh.yigit@intel.com
>>>>>> Cc: dev@dpdk.org; jerinj@marvell.com; viktorin@rehivetech.com;
>>>>>> bruce.richardson@intel.com; Honnappa Nagarahalli
>>>>>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
>>>>>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>>>>>> Subject: Re: [PATCH v6 2/2] net/hns3: refactor SVE code compile
>>>>>> method
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2021/5/20 16:24, Ruifeng Wang wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Chengwen Feng <fengchengwen@huawei.com>
>>>>>>>> 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
>>>>>>>> <Ruifeng.Wang@arm.com>; viktorin@rehivetech.com;
>>>>>>>> bruce.richardson@intel.com; Honnappa Nagarahalli
>>>>>>>> <Honnappa.Nagarahalli@arm.com>; jerinjacobk@gmail.com;
>>>>>>>> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> 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 <fengchengwen@huawei.com>
>>>>>>>> ---
>>>>>>>>  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)
>>>>>>>>  		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) !=
>> ''
>>>>>>>> + 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
>>>>>> 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?
>>>>> I think there is no difference between CC_SVE_ACLE_SUPPORT and
>>>> CC_SVE_SUPPORT when they are used in source code.
>>>>> IMO the same macro name can be used, and it removes redundancy and
>>>> confusion.
>>>>>
>>>>
>>>> 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
>>>> backporting.
>>> 20.11 release has no machine enabled SVE extension.
>>>
>>>>
>>>> Or we could redefine CC_SVE_ACLE_SUPPORT under default
>> machine_args:
>>>>     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.
> 
>>
>> 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'.
> 
>>         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.
> 
>>         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 ?
>>
>>>>         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.
>>>
>>>>         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.
>>>>
>>>>>>
>>>>>>> [1]
>>>>>>> http://patches.dpdk.org/project/dpdk/patch/1621495007-28387-1-
>> git-
>>>>>>> se
>>>>>>> nd
>>>>>>> -email-fengchengwen@huawei.com/
>>>>>>>
>>>>>>>>          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
>>>>>>>
>>>>>>>
>>>>>>> .
>>>>>>>
>>>>>
>>>
>
diff mbox series

Patch

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)
 		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) != '' and cc.check_header('arm_sve.h')
+        cflags += ['-DCC_SVE_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_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