diff mbox series

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

Message ID 1620986039-29475-3-git-send-email-fengchengwen@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
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-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-testing fail Testing issues
ci/iol-abi-testing warning Testing issues
ci/github-robot success github build: passed
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-mellanox-Functional success Functional Testing PASS

Commit Message

Chengwen Feng May 14, 2021, 9:53 a.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 | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Honnappa Nagarahalli May 14, 2021, 2:12 p.m. UTC | #1 
<snip>

> 
> 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>
Looks good to me.
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> ---
>  drivers/net/hns3/hns3_rxtx.c |  2 +-
>  drivers/net/hns3/meson.build | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> 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..8563d70 100644
> --- a/drivers/net/hns3/meson.build
> +++ b/drivers/net/hns3/meson.build
> @@ -35,7 +35,20 @@ deps += ['hash']
> 
>  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
>      sources += files('hns3_rxtx_vec.c')
> +
> +    # 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) != ''
> +        cflags += ['-DCC_SVE_SUPPORT']
>          sources += files('hns3_rxtx_vec_sve.c')
> +    elif cc.has_argument('-march=armv8.2-a+sve')
> +        cflags += ['-DCC_SVE_SUPPORT']
> +        hns3_sve_lib = static_library('hns3_sve_lib',
> +                        'hns3_rxtx_vec_sve.c',
> +                        dependencies: [static_rte_ethdev],
> +                        include_directories: includes,
> +                        c_args: [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 18, 2021, 12:41 p.m. UTC | #2 
Hi, Thomas, Ferruh

This patch is part of the hns3 SVE compilation solution and can work independently. Could you review it ?

Best regards

On 2021/5/14 22:12, Honnappa Nagarahalli wrote:
> <snip>
> 
>>
>> 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>
> Looks good to me.
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> 
>> ---
>>  drivers/net/hns3/hns3_rxtx.c |  2 +-
>>  drivers/net/hns3/meson.build | 13 +++++++++++++
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> 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..8563d70 100644
>> --- a/drivers/net/hns3/meson.build
>> +++ b/drivers/net/hns3/meson.build
>> @@ -35,7 +35,20 @@ deps += ['hash']
>>
>>  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
>>      sources += files('hns3_rxtx_vec.c')
>> +
>> +    # 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) != ''
>> +        cflags += ['-DCC_SVE_SUPPORT']
>>          sources += files('hns3_rxtx_vec_sve.c')
>> +    elif cc.has_argument('-march=armv8.2-a+sve')
>> +        cflags += ['-DCC_SVE_SUPPORT']
>> +        hns3_sve_lib = static_library('hns3_sve_lib',
>> +                        'hns3_rxtx_vec_sve.c',
>> +                        dependencies: [static_rte_ethdev],
>> +                        include_directories: includes,
>> +                        c_args: [cflags, '-march=armv8.2-a+sve'])
>> +        objs += hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
>>      endif
>>  endif
>> --
>> 2.8.1
> 
> 
> .
>
Honnappa Nagarahalli May 18, 2021, 1:11 p.m. UTC | #3 
<snip>
> 
> Hi, Thomas, Ferruh
> 
> This patch is part of the hns3 SVE compilation solution and can work
> independently. Could you review it ?
I believe this patch is targeted for 20.08 release (as 20.05 is already close to completion), is my understanding correct?
If it is targeted for 20.08, it will give us sometime to do few experiments with the generic approach?

> 
> Best regards
> 
> On 2021/5/14 22:12, Honnappa Nagarahalli wrote:
> > <snip>
> >
> >>
> >> 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>
> > Looks good to me.
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> >
> >> ---
> >>  drivers/net/hns3/hns3_rxtx.c |  2 +-  drivers/net/hns3/meson.build |
> >> 13 +++++++++++++
> >>  2 files changed, 14 insertions(+), 1 deletion(-)
> >>
> >> 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..8563d70 100644
> >> --- a/drivers/net/hns3/meson.build
> >> +++ b/drivers/net/hns3/meson.build
> >> @@ -35,7 +35,20 @@ deps += ['hash']
> >>
> >>  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
> >>      sources += files('hns3_rxtx_vec.c')
> >> +
> >> +    # 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) != ''
> >> +        cflags += ['-DCC_SVE_SUPPORT']
> >>          sources += files('hns3_rxtx_vec_sve.c')
> >> +    elif cc.has_argument('-march=armv8.2-a+sve')
> >> +        cflags += ['-DCC_SVE_SUPPORT']
> >> +        hns3_sve_lib = static_library('hns3_sve_lib',
> >> +                        'hns3_rxtx_vec_sve.c',
> >> +                        dependencies: [static_rte_ethdev],
> >> +                        include_directories: includes,
> >> +                        c_args: [cflags, '-march=armv8.2-a+sve'])
> >> +        objs += hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
> >>      endif
> >>  endif
> >> --
> >> 2.8.1
> >
> >
> > .
> >
Honnappa Nagarahalli May 18, 2021, 1:12 p.m. UTC | #4 
> <snip>
> >
> > Hi, Thomas, Ferruh
> >
> > This patch is part of the hns3 SVE compilation solution and can work
> > independently. Could you review it ?
> I believe this patch is targeted for 20.08 release (as 20.05 is already close to
> completion), is my understanding correct?
> If it is targeted for 20.08, it will give us sometime to do few experiments with
> the generic approach?
Apologies, please ignore my comments, I misunderstood the patch. I do not have any issues with this patch.

> 
> >
> > Best regards
> >
> > On 2021/5/14 22:12, Honnappa Nagarahalli wrote:
> > > <snip>
> > >
> > >>
> > >> 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>
> > > Looks good to me.
> > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > >
> > >> ---
> > >>  drivers/net/hns3/hns3_rxtx.c |  2 +-  drivers/net/hns3/meson.build
> > >> |
> > >> 13 +++++++++++++
> > >>  2 files changed, 14 insertions(+), 1 deletion(-)
> > >>
> > >> 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..8563d70 100644
> > >> --- a/drivers/net/hns3/meson.build
> > >> +++ b/drivers/net/hns3/meson.build
> > >> @@ -35,7 +35,20 @@ deps += ['hash']
> > >>
> > >>  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
> > >>      sources += files('hns3_rxtx_vec.c')
> > >> +
> > >> +    # 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) != ''
> > >> +        cflags += ['-DCC_SVE_SUPPORT']
> > >>          sources += files('hns3_rxtx_vec_sve.c')
> > >> +    elif cc.has_argument('-march=armv8.2-a+sve')
> > >> +        cflags += ['-DCC_SVE_SUPPORT']
> > >> +        hns3_sve_lib = static_library('hns3_sve_lib',
> > >> +                        'hns3_rxtx_vec_sve.c',
> > >> +                        dependencies: [static_rte_ethdev],
> > >> +                        include_directories: includes,
> > >> +                        c_args: [cflags, '-march=armv8.2-a+sve'])
> > >> +        objs +=
> > >> + hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
> > >>      endif
> > >>  endif
> > >> --
> > >> 2.8.1
> > >
> > >
> > > .
> > >
Ferruh Yigit May 18, 2021, 1:24 p.m. UTC | #5 
On 5/18/2021 1:41 PM, fengchengwen wrote:
> Hi, Thomas, Ferruh
> 
> This patch is part of the hns3 SVE compilation solution and can work independently. Could you review it ?
> 

Hi Chengwen,

This patch has been missed as being part of the config file set. As far as I
understand you want this change for v20.05, let me check it right now as today
is the -rc4 release day.

> Best regards
> 
> On 2021/5/14 22:12, Honnappa Nagarahalli wrote:
>> <snip>
>>
>>>
>>> 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>
>> Looks good to me.
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>
>>> ---
>>>  drivers/net/hns3/hns3_rxtx.c |  2 +-
>>>  drivers/net/hns3/meson.build | 13 +++++++++++++
>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> 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..8563d70 100644
>>> --- a/drivers/net/hns3/meson.build
>>> +++ b/drivers/net/hns3/meson.build
>>> @@ -35,7 +35,20 @@ deps += ['hash']
>>>
>>>  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
>>>      sources += files('hns3_rxtx_vec.c')
>>> +
>>> +    # 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) != ''
>>> +        cflags += ['-DCC_SVE_SUPPORT']
>>>          sources += files('hns3_rxtx_vec_sve.c')
>>> +    elif cc.has_argument('-march=armv8.2-a+sve')
>>> +        cflags += ['-DCC_SVE_SUPPORT']
>>> +        hns3_sve_lib = static_library('hns3_sve_lib',
>>> +                        'hns3_rxtx_vec_sve.c',
>>> +                        dependencies: [static_rte_ethdev],
>>> +                        include_directories: includes,
>>> +                        c_args: [cflags, '-march=armv8.2-a+sve'])
>>> +        objs += hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
>>>      endif
>>>  endif
>>> --
>>> 2.8.1
>>
>>
>> .
>>
>
Ferruh Yigit May 18, 2021, 2:40 p.m. UTC | #6 
On 5/14/2021 10:53 AM, 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
> 

Hi Chengwen,

As far as I understand from above problem statement, you want to produce a
binary that can run in two different platforms, one supports only NEON
instructions, other supports NEON + SVE.

For this driver should be compiled in a way to support min instruction set,
which is NEON.

There are two build items,

1) hns3_rxtx_vec_sve.c
2) rest of the library

There is already runtime checks to select Rx/Tx functions, so it is safe to
build (1) as long as compiler supports. If the platform doesn't support SVE, the
SVE path won't be selected during runtime.

For (2), it should be build to support NEON only, if it is compiled to support
SVE, it won't run on the platform that only supports NEON.

So, in below, if '__ARM_FEATURE_SVE' is supported, all driver is build with SVE
support, won't this cause a problem on the NEON platform?

What do you think to only keep the else leg of the below check, which is if
compiler supports SVE, set '-DCC_SVE_SUPPORT' flag and only build (1) with SVE flag?

> 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 | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> 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..8563d70 100644
> --- a/drivers/net/hns3/meson.build
> +++ b/drivers/net/hns3/meson.build
> @@ -35,7 +35,20 @@ deps += ['hash']
>  
>  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
>      sources += files('hns3_rxtx_vec.c')
> +
> +    # 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) != ''
> +        cflags += ['-DCC_SVE_SUPPORT']
>          sources += files('hns3_rxtx_vec_sve.c')
> +    elif cc.has_argument('-march=armv8.2-a+sve')
> +        cflags += ['-DCC_SVE_SUPPORT']
> +        hns3_sve_lib = static_library('hns3_sve_lib',
> +                        'hns3_rxtx_vec_sve.c',
> +                        dependencies: [static_rte_ethdev],
> +                        include_directories: includes,
> +                        c_args: [cflags, '-march=armv8.2-a+sve'])
> +        objs += hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
>      endif
>  endif
>
Bruce Richardson May 18, 2021, 3:15 p.m. UTC | #7 
On Tue, May 18, 2021 at 03:40:18PM +0100, Ferruh Yigit wrote:
> On 5/14/2021 10:53 AM, 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
> > 
> 
> Hi Chengwen,
> 
> As far as I understand from above problem statement, you want to produce a
> binary that can run in two different platforms, one supports only NEON
> instructions, other supports NEON + SVE.
> 
> For this driver should be compiled in a way to support min instruction set,
> which is NEON.
> 
> There are two build items,
> 
> 1) hns3_rxtx_vec_sve.c
> 2) rest of the library
> 
> There is already runtime checks to select Rx/Tx functions, so it is safe to
> build (1) as long as compiler supports. If the platform doesn't support SVE, the
> SVE path won't be selected during runtime.
> 
> For (2), it should be build to support NEON only, if it is compiled to support
> SVE, it won't run on the platform that only supports NEON.
> 
> So, in below, if '__ARM_FEATURE_SVE' is supported, all driver is build with SVE
> support, won't this cause a problem on the NEON platform?
> 
In that case, the rest of DPDK is being build with SVE so having one driver
neon-only doesn't really make sense.

Overall, the patch looks to mirror what we do for AVX2/AVX512 support in
the Intel drivers, so looks ok to me.

/Bruce
Honnappa Nagarahalli May 18, 2021, 3:48 p.m. UTC | #8 
<snip>
> 
> On 5/14/2021 10:53 AM, 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
> >
> 
> Hi Chengwen,
> 
> As far as I understand from above problem statement, you want to produce a
> binary that can run in two different platforms, one supports only NEON
> instructions, other supports NEON + SVE.
> 
> For this driver should be compiled in a way to support min instruction set,
> which is NEON.
> 
> There are two build items,
> 
> 1) hns3_rxtx_vec_sve.c
> 2) rest of the library
> 
> There is already runtime checks to select Rx/Tx functions, so it is safe to build
> (1) as long as compiler supports. If the platform doesn't support SVE, the SVE
> path won't be selected during runtime.
> 
> For (2), it should be build to support NEON only, if it is compiled to support
> SVE, it won't run on the platform that only supports NEON.
> 
> So, in below, if '__ARM_FEATURE_SVE' is supported, all driver is build with
> SVE support, won't this cause a problem on the NEON platform?
The first if statement checks if the user has enabled SVE during compilation which indicates that the user will run the binary on a platform that has SVE (the minimum ISA level supported by this binary), hence it is ok to compile all the code with SVE.

If the user has not enabled SVE during compilation which indicates the user might run the binary on a platform that does not have SVE, the second if statement, checks if the compiler supports SVE. If yes, it will compile the SVE version of the driver as well and the run time checks choose the correct version.

> 
> What do you think to only keep the else leg of the below check, which is if
> compiler supports SVE, set '-DCC_SVE_SUPPORT' flag and only build (1) with
> SVE flag?
> 
> > 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 | 13 +++++++++++++
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > 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..8563d70 100644
> > --- a/drivers/net/hns3/meson.build
> > +++ b/drivers/net/hns3/meson.build
> > @@ -35,7 +35,20 @@ deps += ['hash']
> >
> >  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
> >      sources += files('hns3_rxtx_vec.c')
> > +
> > +    # 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) != ''
> > +        cflags += ['-DCC_SVE_SUPPORT']
> >          sources += files('hns3_rxtx_vec_sve.c')
> > +    elif cc.has_argument('-march=armv8.2-a+sve')
> > +        cflags += ['-DCC_SVE_SUPPORT']
> > +        hns3_sve_lib = static_library('hns3_sve_lib',
> > +                        'hns3_rxtx_vec_sve.c',
> > +                        dependencies: [static_rte_ethdev],
> > +                        include_directories: includes,
> > +                        c_args: [cflags, '-march=armv8.2-a+sve'])
> > +        objs += hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
> >      endif
> >  endif
> >
Ferruh Yigit May 18, 2021, 4 p.m. UTC | #9 
On 5/18/2021 4:48 PM, Honnappa Nagarahalli wrote:
> <snip>
>>
>> On 5/14/2021 10:53 AM, 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
>>>
>>
>> Hi Chengwen,
>>
>> As far as I understand from above problem statement, you want to produce a
>> binary that can run in two different platforms, one supports only NEON
>> instructions, other supports NEON + SVE.
>>
>> For this driver should be compiled in a way to support min instruction set,
>> which is NEON.
>>
>> There are two build items,
>>
>> 1) hns3_rxtx_vec_sve.c
>> 2) rest of the library
>>
>> There is already runtime checks to select Rx/Tx functions, so it is safe to build
>> (1) as long as compiler supports. If the platform doesn't support SVE, the SVE
>> path won't be selected during runtime.
>>
>> For (2), it should be build to support NEON only, if it is compiled to support
>> SVE, it won't run on the platform that only supports NEON.
>>
>> So, in below, if '__ARM_FEATURE_SVE' is supported, all driver is build with
>> SVE support, won't this cause a problem on the NEON platform?
> The first if statement checks if the user has enabled SVE during compilation which indicates that the user will run the binary on a platform that has SVE (the minimum ISA level supported by this binary), hence it is ok to compile all the code with SVE.
> 

So it is related to the what user provided (I assume as compiler flag), instead
of host HW capability.

> If the user has not enabled SVE during compilation which indicates the user might run the binary on a platform that does not have SVE, the second if statement, checks if the compiler supports SVE. If yes, it will compile the SVE version of the driver as well and the run time checks choose the correct version.
> 

OK, this sounds good, thanks for clarification.

>>
>> What do you think to only keep the else leg of the below check, which is if
>> compiler supports SVE, set '-DCC_SVE_SUPPORT' flag and only build (1) with
>> SVE flag?
>>
>>> 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 | 13 +++++++++++++
>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> 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..8563d70 100644
>>> --- a/drivers/net/hns3/meson.build
>>> +++ b/drivers/net/hns3/meson.build
>>> @@ -35,7 +35,20 @@ deps += ['hash']
>>>
>>>  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
>>>      sources += files('hns3_rxtx_vec.c')
>>> +
>>> +    # 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) != ''
>>> +        cflags += ['-DCC_SVE_SUPPORT']
>>>          sources += files('hns3_rxtx_vec_sve.c')
>>> +    elif cc.has_argument('-march=armv8.2-a+sve')
>>> +        cflags += ['-DCC_SVE_SUPPORT']
>>> +        hns3_sve_lib = static_library('hns3_sve_lib',
>>> +                        'hns3_rxtx_vec_sve.c',
>>> +                        dependencies: [static_rte_ethdev],
>>> +                        include_directories: includes,
>>> +                        c_args: [cflags, '-march=armv8.2-a+sve'])
>>> +        objs += hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
>>>      endif
>>>  endif
>>>
>
Ferruh Yigit May 18, 2021, 4:12 p.m. UTC | #10 
On 5/18/2021 4:15 PM, Bruce Richardson wrote:
> On Tue, May 18, 2021 at 03:40:18PM +0100, Ferruh Yigit wrote:
>> On 5/14/2021 10:53 AM, 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
>>>
>>
>> Hi Chengwen,
>>
>> As far as I understand from above problem statement, you want to produce a
>> binary that can run in two different platforms, one supports only NEON
>> instructions, other supports NEON + SVE.
>>
>> For this driver should be compiled in a way to support min instruction set,
>> which is NEON.
>>
>> There are two build items,
>>
>> 1) hns3_rxtx_vec_sve.c
>> 2) rest of the library
>>
>> There is already runtime checks to select Rx/Tx functions, so it is safe to
>> build (1) as long as compiler supports. If the platform doesn't support SVE, the
>> SVE path won't be selected during runtime.
>>
>> For (2), it should be build to support NEON only, if it is compiled to support
>> SVE, it won't run on the platform that only supports NEON.
>>
>> So, in below, if '__ARM_FEATURE_SVE' is supported, all driver is build with SVE
>> support, won't this cause a problem on the NEON platform?
>>
> In that case, the rest of DPDK is being build with SVE so having one driver
> neon-only doesn't really make sense.
> 
> Overall, the patch looks to mirror what we do for AVX2/AVX512 support in
> the Intel drivers, so looks ok to me.
> 

I agree there is no point to make driver specific change if whole DPDK build
with SVE, and that seems controlled by user as Honnappa clarified.
So I will proceed with patch, thanks.
Honnappa Nagarahalli May 18, 2021, 4:12 p.m. UTC | #11 
> > <snip>
> >>
> >> On 5/14/2021 10:53 AM, 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
> >>>
> >>
> >> Hi Chengwen,
> >>
> >> As far as I understand from above problem statement, you want to
> >> produce a binary that can run in two different platforms, one
> >> supports only NEON instructions, other supports NEON + SVE.
> >>
> >> For this driver should be compiled in a way to support min
> >> instruction set, which is NEON.
> >>
> >> There are two build items,
> >>
> >> 1) hns3_rxtx_vec_sve.c
> >> 2) rest of the library
> >>
> >> There is already runtime checks to select Rx/Tx functions, so it is
> >> safe to build
> >> (1) as long as compiler supports. If the platform doesn't support
> >> SVE, the SVE path won't be selected during runtime.
> >>
> >> For (2), it should be build to support NEON only, if it is compiled
> >> to support SVE, it won't run on the platform that only supports NEON.
> >>
> >> So, in below, if '__ARM_FEATURE_SVE' is supported, all driver is
> >> build with SVE support, won't this cause a problem on the NEON platform?
> > The first if statement checks if the user has enabled SVE during compilation
> which indicates that the user will run the binary on a platform that has SVE
> (the minimum ISA level supported by this binary), hence it is ok to compile all
> the code with SVE.
> >
> 
> So it is related to the what user provided (I assume as compiler flag), instead
> of host HW capability.
It is the HW host capability as provided in the compiler flag. It is coming from config/arm/meson.build.

> 
> > If the user has not enabled SVE during compilation which indicates the user
> might run the binary on a platform that does not have SVE, the second if
> statement, checks if the compiler supports SVE. If yes, it will compile the SVE
> version of the driver as well and the run time checks choose the correct
> version.
> >
> 
> OK, this sounds good, thanks for clarification.
> 
> >>
> >> What do you think to only keep the else leg of the below check, which
> >> is if compiler supports SVE, set '-DCC_SVE_SUPPORT' flag and only
> >> build (1) with SVE flag?
> >>
> >>> 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
> >>> | 13 +++++++++++++
> >>>  2 files changed, 14 insertions(+), 1 deletion(-)
> >>>
> >>> 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..8563d70 100644
> >>> --- a/drivers/net/hns3/meson.build
> >>> +++ b/drivers/net/hns3/meson.build
> >>> @@ -35,7 +35,20 @@ deps += ['hash']
> >>>
> >>>  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
> >>>      sources += files('hns3_rxtx_vec.c')
> >>> +
> >>> +    # 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) != ''
> >>> +        cflags += ['-DCC_SVE_SUPPORT']
> >>>          sources += files('hns3_rxtx_vec_sve.c')
> >>> +    elif cc.has_argument('-march=armv8.2-a+sve')
> >>> +        cflags += ['-DCC_SVE_SUPPORT']
> >>> +        hns3_sve_lib = static_library('hns3_sve_lib',
> >>> +                        'hns3_rxtx_vec_sve.c',
> >>> +                        dependencies: [static_rte_ethdev],
> >>> +                        include_directories: includes,
> >>> +                        c_args: [cflags, '-march=armv8.2-a+sve'])
> >>> +        objs += hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
> >>>      endif
> >>>  endif
> >>>
> >
Ferruh Yigit May 18, 2021, 4:27 p.m. UTC | #12 
On 5/14/2021 3:12 PM, Honnappa Nagarahalli wrote:
> <snip>
> 
>>
>> 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>
> Looks good to me.
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> 

Applied to dpdk-next-net/main, thanks.

(Only this patch, 2/2, applied, not whole set)
Ferruh Yigit May 18, 2021, 4:37 p.m. UTC | #13 
On 5/18/2021 5:12 PM, Honnappa Nagarahalli wrote:
>>> <snip>
>>>>
>>>> On 5/14/2021 10:53 AM, 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
>>>>>
>>>>
>>>> Hi Chengwen,
>>>>
>>>> As far as I understand from above problem statement, you want to
>>>> produce a binary that can run in two different platforms, one
>>>> supports only NEON instructions, other supports NEON + SVE.
>>>>
>>>> For this driver should be compiled in a way to support min
>>>> instruction set, which is NEON.
>>>>
>>>> There are two build items,
>>>>
>>>> 1) hns3_rxtx_vec_sve.c
>>>> 2) rest of the library
>>>>
>>>> There is already runtime checks to select Rx/Tx functions, so it is
>>>> safe to build
>>>> (1) as long as compiler supports. If the platform doesn't support
>>>> SVE, the SVE path won't be selected during runtime.
>>>>
>>>> For (2), it should be build to support NEON only, if it is compiled
>>>> to support SVE, it won't run on the platform that only supports NEON.
>>>>
>>>> So, in below, if '__ARM_FEATURE_SVE' is supported, all driver is
>>>> build with SVE support, won't this cause a problem on the NEON platform?
>>> The first if statement checks if the user has enabled SVE during compilation
>> which indicates that the user will run the binary on a platform that has SVE
>> (the minimum ISA level supported by this binary), hence it is ok to compile all
>> the code with SVE.
>>>
>>
>> So it is related to the what user provided (I assume as compiler flag), instead
>> of host HW capability.
> It is the HW host capability as provided in the compiler flag. It is coming from config/arm/meson.build.
> 

Is this patch has dependency to 1/2, that updates 'config/arm/meson.build'?

What I understand is, if user provides compiler argument to request SVE,
something like '-march=armv8.2-a+sve', and host HW supports it, whole driver
will be built with SVE support.

If user not request SVE, driver won't be compiled with SVE support even if HW
support it, but only 'hns3_rxtx_vec_sve.c' will be compiled if compiler supports
SVE.

Is above correct and does it have any dependency to first patch, I thought this
is independent from first patch.


>>
>>> If the user has not enabled SVE during compilation which indicates the user
>> might run the binary on a platform that does not have SVE, the second if
>> statement, checks if the compiler supports SVE. If yes, it will compile the SVE
>> version of the driver as well and the run time checks choose the correct
>> version.
>>>
>>
>> OK, this sounds good, thanks for clarification.
>>
>>>>
>>>> What do you think to only keep the else leg of the below check, which
>>>> is if compiler supports SVE, set '-DCC_SVE_SUPPORT' flag and only
>>>> build (1) with SVE flag?
>>>>
>>>>> 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
>>>>> | 13 +++++++++++++
>>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>>
>>>>> 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..8563d70 100644
>>>>> --- a/drivers/net/hns3/meson.build
>>>>> +++ b/drivers/net/hns3/meson.build
>>>>> @@ -35,7 +35,20 @@ deps += ['hash']
>>>>>
>>>>>  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
>>>>>      sources += files('hns3_rxtx_vec.c')
>>>>> +
>>>>> +    # 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) != ''
>>>>> +        cflags += ['-DCC_SVE_SUPPORT']
>>>>>          sources += files('hns3_rxtx_vec_sve.c')
>>>>> +    elif cc.has_argument('-march=armv8.2-a+sve')
>>>>> +        cflags += ['-DCC_SVE_SUPPORT']
>>>>> +        hns3_sve_lib = static_library('hns3_sve_lib',
>>>>> +                        'hns3_rxtx_vec_sve.c',
>>>>> +                        dependencies: [static_rte_ethdev],
>>>>> +                        include_directories: includes,
>>>>> +                        c_args: [cflags, '-march=armv8.2-a+sve'])
>>>>> +        objs += hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
>>>>>      endif
>>>>>  endif
>>>>>
>>>
>
Chengwen Feng May 19, 2021, 12:18 a.m. UTC | #14 
On 2021/5/19 0:37, Ferruh Yigit wrote:
> On 5/18/2021 5:12 PM, Honnappa Nagarahalli wrote:
>>>> <snip>
>>>>>
>>>>> On 5/14/2021 10:53 AM, 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
>>>>>>
>>>>>
>>>>> Hi Chengwen,
>>>>>
>>>>> As far as I understand from above problem statement, you want to
>>>>> produce a binary that can run in two different platforms, one
>>>>> supports only NEON instructions, other supports NEON + SVE.
>>>>>
>>>>> For this driver should be compiled in a way to support min
>>>>> instruction set, which is NEON.
>>>>>
>>>>> There are two build items,
>>>>>
>>>>> 1) hns3_rxtx_vec_sve.c
>>>>> 2) rest of the library
>>>>>
>>>>> There is already runtime checks to select Rx/Tx functions, so it is
>>>>> safe to build
>>>>> (1) as long as compiler supports. If the platform doesn't support
>>>>> SVE, the SVE path won't be selected during runtime.
>>>>>
>>>>> For (2), it should be build to support NEON only, if it is compiled
>>>>> to support SVE, it won't run on the platform that only supports NEON.
>>>>>
>>>>> So, in below, if '__ARM_FEATURE_SVE' is supported, all driver is
>>>>> build with SVE support, won't this cause a problem on the NEON platform?
>>>> The first if statement checks if the user has enabled SVE during compilation
>>> which indicates that the user will run the binary on a platform that has SVE
>>> (the minimum ISA level supported by this binary), hence it is ok to compile all
>>> the code with SVE.
>>>>
>>>
>>> So it is related to the what user provided (I assume as compiler flag), instead
>>> of host HW capability.
>> It is the HW host capability as provided in the compiler flag. It is coming from config/arm/meson.build.
>>
> 
> Is this patch has dependency to 1/2, that updates 'config/arm/meson.build'?
> 
> What I understand is, if user provides compiler argument to request SVE,
> something like '-march=armv8.2-a+sve', and host HW supports it, whole driver
> will be built with SVE support.
> 
> If user not request SVE, driver won't be compiled with SVE support even if HW
> support it, but only 'hns3_rxtx_vec_sve.c' will be compiled if compiler supports
> SVE.
> 
> Is above correct and does it have any dependency to first patch, I thought this
> is independent from first patch.
> 

Yes, you are right, it's independent from first patch.

> 
>>>
>>>> If the user has not enabled SVE during compilation which indicates the user
>>> might run the binary on a platform that does not have SVE, the second if
>>> statement, checks if the compiler supports SVE. If yes, it will compile the SVE
>>> version of the driver as well and the run time checks choose the correct
>>> version.
>>>>
>>>
>>> OK, this sounds good, thanks for clarification.
>>>
>>>>>
>>>>> What do you think to only keep the else leg of the below check, which
>>>>> is if compiler supports SVE, set '-DCC_SVE_SUPPORT' flag and only
>>>>> build (1) with SVE flag?
>>>>>
>>>>>> 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
>>>>>> | 13 +++++++++++++
>>>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> 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..8563d70 100644
>>>>>> --- a/drivers/net/hns3/meson.build
>>>>>> +++ b/drivers/net/hns3/meson.build
>>>>>> @@ -35,7 +35,20 @@ deps += ['hash']
>>>>>>
>>>>>>  if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
>>>>>>      sources += files('hns3_rxtx_vec.c')
>>>>>> +
>>>>>> +    # 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) != ''
>>>>>> +        cflags += ['-DCC_SVE_SUPPORT']
>>>>>>          sources += files('hns3_rxtx_vec_sve.c')
>>>>>> +    elif cc.has_argument('-march=armv8.2-a+sve')
>>>>>> +        cflags += ['-DCC_SVE_SUPPORT']
>>>>>> +        hns3_sve_lib = static_library('hns3_sve_lib',
>>>>>> +                        'hns3_rxtx_vec_sve.c',
>>>>>> +                        dependencies: [static_rte_ethdev],
>>>>>> +                        include_directories: includes,
>>>>>> +                        c_args: [cflags, '-march=armv8.2-a+sve'])
>>>>>> +        objs += hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
>>>>>>      endif
>>>>>>  endif
>>>>>>
>>>>
>>
> 
> 
> .
>
Chengwen Feng May 19, 2021, 12:23 a.m. UTC | #15 
Thanks a lot, Ferruh & Honnappa.

On 2021/5/19 0:27, Ferruh Yigit wrote:
> On 5/14/2021 3:12 PM, Honnappa Nagarahalli wrote:
>> <snip>
>>
>>>
>>> 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>
>> Looks good to me.
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>
> 
> Applied to dpdk-next-net/main, thanks.
> 
> (Only this patch, 2/2, applied, not whole set)
> 
> .
>
David Marchand May 19, 2021, 8:08 a.m. UTC | #16 
On Tue, May 18, 2021 at 6:28 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 5/14/2021 3:12 PM, Honnappa Nagarahalli wrote:
> > <snip>
> >
> >>
> >> 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>
> > Looks good to me.
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> >
>
> Applied to dpdk-next-net/main, thanks.
>
> (Only this patch, 2/2, applied, not whole set)

UNH caught a build regression on next-net and I think this is due to this patch:
https://lab.dpdk.org/results/dashboard/tarballs/15259/

FAILED: drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o
ccache cc -Idrivers/net/hns3/libhns3_sve_lib.a.p -Idrivers/net/hns3
-I../drivers/net/hns3 -Ilib/ethdev -I../lib/ethdev -I. -I.. -Iconfig
-I../config -Ilib/eal/include -I../lib/eal/include
-Ilib/eal/linux/include -I../lib/eal/linux/include
-Ilib/eal/arm/include -I../lib/eal/arm/include -Ilib/eal/common
-I../lib/eal/common -Ilib/eal -I../lib/eal -Ilib/kvargs
-I../lib/kvargs -Ilib/metrics -I../lib/metrics -Ilib/telemetry
-I../lib/telemetry -Ilib/net -I../lib/net -Ilib/mbuf -I../lib/mbuf
-Ilib/mempool -I../lib/mempool -Ilib/ring -I../lib/ring -Ilib/meter
-I../lib/meter -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64
-Wall -Winvalid-pch -Werror -O3 -include rte_config.h -Wextra
-Wcast-qual -Wdeprecated -Wformat -Wformat-nonliteral
-Wformat-security -Wmissing-declarations -Wmissing-prototypes
-Wnested-externs -Wold-style-definition -Wpointer-arith -Wsign-compare
-Wstrict-prototypes -Wundef -Wwrite-strings
-Wno-address-of-packed-member -Wno-packed-not-aligned
-Wno-missing-field-initializers -D_GNU_SOURCE -fPIC
-march=armv8.1-a+crc+crypto -mcpu=thunderx2t99
-DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation
-DCC_SVE_SUPPORT -march=armv8.2-a+sve -MD -MQ
drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -MF
drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o.d -o
drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -c
../drivers/net/hns3/hns3_rxtx_vec_sve.c
cc1: error: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’
switch [-Werror]
../drivers/net/hns3/hns3_rxtx_vec_sve.c:5:10: fatal error: arm_sve.h:
No such file or directory
    5 | #include <arm_sve.h>
      |          ^~~~~~~~~~~
cc1: all warnings being treated as errors
compilation terminated.


I see a similar issue on my fc32, cross compiling with following gcc:
$ 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
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[74/503] Compiling C object
drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o
FAILED: drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o
aarch64-linux-gnu-gcc -Idrivers/net/hns3/libhns3_sve_lib.a.p
-Idrivers/net/hns3 -I../../dpdk/drivers/net/hns3 -Ilib/ethdev
-I../../dpdk/lib/ethdev -I. -I../../dpdk -Iconfig -I../../dpdk/config
-Ilib/eal/include -I../../dpdk/lib/eal/include -Ilib/eal/linux/include
-I../../dpdk/lib/eal/linux/include -Ilib/eal/arm/include
-I../../dpdk/lib/eal/arm/include -Ilib/eal/common
-I../../dpdk/lib/eal/common -Ilib/eal -I../../dpdk/lib/eal
-Ilib/kvargs -I../../dpdk/lib/kvargs -Ilib/metrics
-I../../dpdk/lib/metrics -Ilib/telemetry -I../../dpdk/lib/telemetry
-Ilib/net -I../../dpdk/lib/net -Ilib/mbuf -I../../dpdk/lib/mbuf
-Ilib/mempool -I../../dpdk/lib/mempool -Ilib/ring
-I../../dpdk/lib/ring -Ilib/meter -I../../dpdk/lib/meter
-fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall
-Winvalid-pch -Werror -O2 -g -include rte_config.h -Wextra -Wcast-qual
-Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security
-Wmissing-declarations -Wmissing-prototypes -Wnested-externs
-Wold-style-definition -Wpointer-arith -Wsign-compare
-Wstrict-prototypes -Wundef -Wwrite-strings -Wno-packed-not-aligned
-Wno-missing-field-initializers -D_GNU_SOURCE -fPIC -march=armv8-a+crc
-DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation
-DCC_SVE_SUPPORT -march=armv8.2-a+sve -MD -MQ
drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -MF
drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o.d -o
drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -c
../../dpdk/drivers/net/hns3/hns3_rxtx_vec_sve.c
../../dpdk/drivers/net/hns3/hns3_rxtx_vec_sve.c:5:10: fatal error:
arm_sve.h: No such file or directory
 #include <arm_sve.h>
          ^~~~~~~~~~~
compilation terminated.
Ferruh Yigit May 19, 2021, 9:27 a.m. UTC | #17 
On 5/19/2021 9:08 AM, David Marchand wrote:
> On Tue, May 18, 2021 at 6:28 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> On 5/14/2021 3:12 PM, Honnappa Nagarahalli wrote:
>>> <snip>
>>>
>>>>
>>>> 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>
>>> Looks good to me.
>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>>
>>
>> Applied to dpdk-next-net/main, thanks.
>>
>> (Only this patch, 2/2, applied, not whole set)
> 
> UNH caught a build regression on next-net and I think this is due to this patch:
> https://lab.dpdk.org/results/dashboard/tarballs/15259/
> 

Thanks David for heads up, yet it the reason of the error seems this patch, so I
will drop it from the next-net.


A few details from the error:

Default used compiler flag does not request SVE support, it is:
  Compiler for C supports arguments -march=armv8.1-a+crc+crypto: YES
  Compiler for C supports arguments -mcpu=thunderx2t99: YES
  Message: Using machine args: ['-march=armv8.1-a+crc+crypto', '-mcpu=thunderx2t99']

It is detected that compiler can support SVE:
  Compiler for C supports arguments -march=armv8.2-a+sve: YES

Because of the SVE support, driver tries to build 'hns3_rxtx_vec_sve.c' with SVE
support, even user doesn't request SVE, this is the design in the patch, build
command (stripped):
  -march=armv8.1-a+crc+crypto -mcpu=thunderx2t99 -DCC_SVE_SUPPORT
-march=armv8.2-a+sve ../drivers/net/hns3/hns3_rxtx_vec_sve.c

There are two errors:

1) appended '-march=armv8.2-a+sve' compiler argument to build
'hns3_rxtx_vec_sve.c' is conflicting with rest of the command:
  cc1: error: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’ switch
[-Werror]


2) SVE header is missing. When compiler supports the SVE, not sure why the
header is missing:
  ../drivers/net/hns3/hns3_rxtx_vec_sve.c:5:10: fatal error: arm_sve.h: No such
file or directory
    5 | #include <arm_sve.h>
      |          ^~~~~~~~~~~


> FAILED: drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o
> ccache cc -Idrivers/net/hns3/libhns3_sve_lib.a.p -Idrivers/net/hns3
> -I../drivers/net/hns3 -Ilib/ethdev -I../lib/ethdev -I. -I.. -Iconfig
> -I../config -Ilib/eal/include -I../lib/eal/include
> -Ilib/eal/linux/include -I../lib/eal/linux/include
> -Ilib/eal/arm/include -I../lib/eal/arm/include -Ilib/eal/common
> -I../lib/eal/common -Ilib/eal -I../lib/eal -Ilib/kvargs
> -I../lib/kvargs -Ilib/metrics -I../lib/metrics -Ilib/telemetry
> -I../lib/telemetry -Ilib/net -I../lib/net -Ilib/mbuf -I../lib/mbuf
> -Ilib/mempool -I../lib/mempool -Ilib/ring -I../lib/ring -Ilib/meter
> -I../lib/meter -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64
> -Wall -Winvalid-pch -Werror -O3 -include rte_config.h -Wextra
> -Wcast-qual -Wdeprecated -Wformat -Wformat-nonliteral
> -Wformat-security -Wmissing-declarations -Wmissing-prototypes
> -Wnested-externs -Wold-style-definition -Wpointer-arith -Wsign-compare
> -Wstrict-prototypes -Wundef -Wwrite-strings
> -Wno-address-of-packed-member -Wno-packed-not-aligned
> -Wno-missing-field-initializers -D_GNU_SOURCE -fPIC
> -march=armv8.1-a+crc+crypto -mcpu=thunderx2t99
> -DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation
> -DCC_SVE_SUPPORT -march=armv8.2-a+sve -MD -MQ
> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -MF
> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o.d -o
> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -c
> ../drivers/net/hns3/hns3_rxtx_vec_sve.c
> cc1: error: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’
> switch [-Werror]
> ../drivers/net/hns3/hns3_rxtx_vec_sve.c:5:10: fatal error: arm_sve.h:
> No such file or directory
>     5 | #include <arm_sve.h>
>       |          ^~~~~~~~~~~
> cc1: all warnings being treated as errors
> compilation terminated.
> 
> 
> I see a similar issue on my fc32, cross compiling with following gcc:
> $ 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
> Copyright (C) 2018 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> [74/503] Compiling C object
> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o
> FAILED: drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o
> aarch64-linux-gnu-gcc -Idrivers/net/hns3/libhns3_sve_lib.a.p
> -Idrivers/net/hns3 -I../../dpdk/drivers/net/hns3 -Ilib/ethdev
> -I../../dpdk/lib/ethdev -I. -I../../dpdk -Iconfig -I../../dpdk/config
> -Ilib/eal/include -I../../dpdk/lib/eal/include -Ilib/eal/linux/include
> -I../../dpdk/lib/eal/linux/include -Ilib/eal/arm/include
> -I../../dpdk/lib/eal/arm/include -Ilib/eal/common
> -I../../dpdk/lib/eal/common -Ilib/eal -I../../dpdk/lib/eal
> -Ilib/kvargs -I../../dpdk/lib/kvargs -Ilib/metrics
> -I../../dpdk/lib/metrics -Ilib/telemetry -I../../dpdk/lib/telemetry
> -Ilib/net -I../../dpdk/lib/net -Ilib/mbuf -I../../dpdk/lib/mbuf
> -Ilib/mempool -I../../dpdk/lib/mempool -Ilib/ring
> -I../../dpdk/lib/ring -Ilib/meter -I../../dpdk/lib/meter
> -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall
> -Winvalid-pch -Werror -O2 -g -include rte_config.h -Wextra -Wcast-qual
> -Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security
> -Wmissing-declarations -Wmissing-prototypes -Wnested-externs
> -Wold-style-definition -Wpointer-arith -Wsign-compare
> -Wstrict-prototypes -Wundef -Wwrite-strings -Wno-packed-not-aligned
> -Wno-missing-field-initializers -D_GNU_SOURCE -fPIC -march=armv8-a+crc
> -DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation
> -DCC_SVE_SUPPORT -march=armv8.2-a+sve -MD -MQ
> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -MF
> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o.d -o
> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -c
> ../../dpdk/drivers/net/hns3/hns3_rxtx_vec_sve.c
> ../../dpdk/drivers/net/hns3/hns3_rxtx_vec_sve.c:5:10: fatal error:
> arm_sve.h: No such file or directory
>  #include <arm_sve.h>
>           ^~~~~~~~~~~
> compilation terminated.
> 
>
Chengwen Feng May 19, 2021, 12:16 p.m. UTC | #18 
On 2021/5/19 17:27, Ferruh Yigit wrote:
> On 5/19/2021 9:08 AM, David Marchand wrote:
>> On Tue, May 18, 2021 at 6:28 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>>
>>> On 5/14/2021 3:12 PM, Honnappa Nagarahalli wrote:
>>>> <snip>
>>>>
>>>>>
>>>>> 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>
>>>> Looks good to me.
>>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>>>
>>>
>>> Applied to dpdk-next-net/main, thanks.
>>>
>>> (Only this patch, 2/2, applied, not whole set)
>>
>> UNH caught a build regression on next-net and I think this is due to this patch:
>> https://lab.dpdk.org/results/dashboard/tarballs/15259/
>>
> 
> Thanks David for heads up, yet it the reason of the error seems this patch, so I
> will drop it from the next-net.
> 
> 
> A few details from the error:
> 
> Default used compiler flag does not request SVE support, it is:
>   Compiler for C supports arguments -march=armv8.1-a+crc+crypto: YES
>   Compiler for C supports arguments -mcpu=thunderx2t99: YES
>   Message: Using machine args: ['-march=armv8.1-a+crc+crypto', '-mcpu=thunderx2t99']
> 
> It is detected that compiler can support SVE:
>   Compiler for C supports arguments -march=armv8.2-a+sve: YES
> 
> Because of the SVE support, driver tries to build 'hns3_rxtx_vec_sve.c' with SVE
> support, even user doesn't request SVE, this is the design in the patch, build
> command (stripped):
>   -march=armv8.1-a+crc+crypto -mcpu=thunderx2t99 -DCC_SVE_SUPPORT
> -march=armv8.2-a+sve ../drivers/net/hns3/hns3_rxtx_vec_sve.c
> 
> There are two errors:
> 
> 1) appended '-march=armv8.2-a+sve' compiler argument to build
> 'hns3_rxtx_vec_sve.c' is conflicting with rest of the command:
>   cc1: error: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’ switch
> [-Werror]
> 
> 
> 2) SVE header is missing. When compiler supports the SVE, not sure why the
> header is missing:
>   ../drivers/net/hns3/hns3_rxtx_vec_sve.c:5:10: fatal error: arm_sve.h: No such
> file or directory
>     5 | #include <arm_sve.h>
>       |          ^~~~~~~~~~~
> 

Sorry to introduce compilation errors.
Already fix in new patch: [dpdk-dev] [PATCH v2] net/hns3: fix compile error with gcc8.3 and thunderx2
@David could you help test?


We also found compile error with gcc8.3 with arm64_kunpeng930_linux_gcc (-march=-march=armv8.2-a+crypto+sve):
  In file included from ../dpdk-next-net/lib/eal/common/eal_common_options.c:38:
  ../dpdk-next-net/lib/eal/arm/include/rte_vect.h:13:10: fatal error: arm_sve.h: No such file or directory
   #include <arm_sve.h>
          ^~~~~~~~~~~
  compilation terminated.
  [126/2250] Compiling C object lib/librte_net.a.p/net_rte_net_crc.c.o

the corresponding code:
  #ifdef __ARM_FEATURE_SVE
  #include <arm_sve.h>
  #endif

this is because gcc8.3 defined __ARM_FEATURE_SVE but it can't compile ACLE SVE code.
I will fix it later in 21.08

Best regards

> 
>> FAILED: drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o
>> ccache cc -Idrivers/net/hns3/libhns3_sve_lib.a.p -Idrivers/net/hns3
>> -I../drivers/net/hns3 -Ilib/ethdev -I../lib/ethdev -I. -I.. -Iconfig
>> -I../config -Ilib/eal/include -I../lib/eal/include
>> -Ilib/eal/linux/include -I../lib/eal/linux/include
>> -Ilib/eal/arm/include -I../lib/eal/arm/include -Ilib/eal/common
>> -I../lib/eal/common -Ilib/eal -I../lib/eal -Ilib/kvargs
>> -I../lib/kvargs -Ilib/metrics -I../lib/metrics -Ilib/telemetry
>> -I../lib/telemetry -Ilib/net -I../lib/net -Ilib/mbuf -I../lib/mbuf
>> -Ilib/mempool -I../lib/mempool -Ilib/ring -I../lib/ring -Ilib/meter
>> -I../lib/meter -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64
>> -Wall -Winvalid-pch -Werror -O3 -include rte_config.h -Wextra
>> -Wcast-qual -Wdeprecated -Wformat -Wformat-nonliteral
>> -Wformat-security -Wmissing-declarations -Wmissing-prototypes
>> -Wnested-externs -Wold-style-definition -Wpointer-arith -Wsign-compare
>> -Wstrict-prototypes -Wundef -Wwrite-strings
>> -Wno-address-of-packed-member -Wno-packed-not-aligned
>> -Wno-missing-field-initializers -D_GNU_SOURCE -fPIC
>> -march=armv8.1-a+crc+crypto -mcpu=thunderx2t99
>> -DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation
>> -DCC_SVE_SUPPORT -march=armv8.2-a+sve -MD -MQ
>> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -MF
>> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o.d -o
>> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -c
>> ../drivers/net/hns3/hns3_rxtx_vec_sve.c
>> cc1: error: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’
>> switch [-Werror]
>> ../drivers/net/hns3/hns3_rxtx_vec_sve.c:5:10: fatal error: arm_sve.h:
>> No such file or directory
>>     5 | #include <arm_sve.h>
>>       |          ^~~~~~~~~~~
>> cc1: all warnings being treated as errors
>> compilation terminated.
>>
>>
>> I see a similar issue on my fc32, cross compiling with following gcc:
>> $ 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
>> Copyright (C) 2018 Free Software Foundation, Inc.
>> This is free software; see the source for copying conditions.  There is NO
>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>
>> [74/503] Compiling C object
>> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o
>> FAILED: drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o
>> aarch64-linux-gnu-gcc -Idrivers/net/hns3/libhns3_sve_lib.a.p
>> -Idrivers/net/hns3 -I../../dpdk/drivers/net/hns3 -Ilib/ethdev
>> -I../../dpdk/lib/ethdev -I. -I../../dpdk -Iconfig -I../../dpdk/config
>> -Ilib/eal/include -I../../dpdk/lib/eal/include -Ilib/eal/linux/include
>> -I../../dpdk/lib/eal/linux/include -Ilib/eal/arm/include
>> -I../../dpdk/lib/eal/arm/include -Ilib/eal/common
>> -I../../dpdk/lib/eal/common -Ilib/eal -I../../dpdk/lib/eal
>> -Ilib/kvargs -I../../dpdk/lib/kvargs -Ilib/metrics
>> -I../../dpdk/lib/metrics -Ilib/telemetry -I../../dpdk/lib/telemetry
>> -Ilib/net -I../../dpdk/lib/net -Ilib/mbuf -I../../dpdk/lib/mbuf
>> -Ilib/mempool -I../../dpdk/lib/mempool -Ilib/ring
>> -I../../dpdk/lib/ring -Ilib/meter -I../../dpdk/lib/meter
>> -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall
>> -Winvalid-pch -Werror -O2 -g -include rte_config.h -Wextra -Wcast-qual
>> -Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security
>> -Wmissing-declarations -Wmissing-prototypes -Wnested-externs
>> -Wold-style-definition -Wpointer-arith -Wsign-compare
>> -Wstrict-prototypes -Wundef -Wwrite-strings -Wno-packed-not-aligned
>> -Wno-missing-field-initializers -D_GNU_SOURCE -fPIC -march=armv8-a+crc
>> -DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation
>> -DCC_SVE_SUPPORT -march=armv8.2-a+sve -MD -MQ
>> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -MF
>> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o.d -o
>> drivers/net/hns3/libhns3_sve_lib.a.p/hns3_rxtx_vec_sve.c.o -c
>> ../../dpdk/drivers/net/hns3/hns3_rxtx_vec_sve.c
>> ../../dpdk/drivers/net/hns3/hns3_rxtx_vec_sve.c:5:10: fatal error:
>> arm_sve.h: No such file or directory
>>  #include <arm_sve.h>
>>           ^~~~~~~~~~~
>> compilation terminated.
>>
>>
> 
> 
> .
>
David Marchand May 19, 2021, 12:37 p.m. UTC | #19 
On Wed, May 19, 2021 at 2:17 PM fengchengwen <fengchengwen@huawei.com> wrote:
> We also found compile error with gcc8.3 with arm64_kunpeng930_linux_gcc (-march=-march=armv8.2-a+crypto+sve):
>   In file included from ../dpdk-next-net/lib/eal/common/eal_common_options.c:38:
>   ../dpdk-next-net/lib/eal/arm/include/rte_vect.h:13:10: fatal error: arm_sve.h: No such file or directory
>    #include <arm_sve.h>
>           ^~~~~~~~~~~
>   compilation terminated.
>   [126/2250] Compiling C object lib/librte_net.a.p/net_rte_net_crc.c.o
>
> the corresponding code:
>   #ifdef __ARM_FEATURE_SVE
>   #include <arm_sve.h>
>   #endif
>
> this is because gcc8.3 defined __ARM_FEATURE_SVE but it can't compile ACLE SVE code.

Afaiu, gcc 8.3 does not define __ARM_FEATURE_SVE:

$ 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
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.



$ aarch64-linux-gnu-gcc - -pipe -E -P -D_FILE_OFFSET_BITS=64 -P -O0
-march=armv8-a+crc <<EOF
>         #ifndef __ARM_FEATURE_SVE
>         # define __ARM_FEATURE_SVE
>         #endif
>         "MESON_GET_DEFINE_DELIMITER"
> __ARM_FEATURE_SVE
> EOF
        "MESON_GET_DEFINE_DELIMITER"
Chengwen Feng May 19, 2021, 1:35 p.m. UTC | #20 
On 2021/5/19 20:37, David Marchand wrote:
> On Wed, May 19, 2021 at 2:17 PM fengchengwen <fengchengwen@huawei.com> wrote:
>> We also found compile error with gcc8.3 with arm64_kunpeng930_linux_gcc (-march=-march=armv8.2-a+crypto+sve):
>>   In file included from ../dpdk-next-net/lib/eal/common/eal_common_options.c:38:
>>   ../dpdk-next-net/lib/eal/arm/include/rte_vect.h:13:10: fatal error: arm_sve.h: No such file or directory
>>    #include <arm_sve.h>
>>           ^~~~~~~~~~~
>>   compilation terminated.
>>   [126/2250] Compiling C object lib/librte_net.a.p/net_rte_net_crc.c.o
>>
>> the corresponding code:
>>   #ifdef __ARM_FEATURE_SVE
>>   #include <arm_sve.h>
>>   #endif
>>
>> this is because gcc8.3 defined __ARM_FEATURE_SVE but it can't compile ACLE SVE code.
> 
> Afaiu, gcc 8.3 does not define __ARM_FEATURE_SVE:
> 
> $ 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
> Copyright (C) 2018 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> 
> 
> $ aarch64-linux-gnu-gcc - -pipe -E -P -D_FILE_OFFSET_BITS=64 -P -O0
> -march=armv8-a+crc <<EOF
>>         #ifndef __ARM_FEATURE_SVE
>>         # define __ARM_FEATURE_SVE
>>         #endif
>>         "MESON_GET_DEFINE_DELIMITER"
>> __ARM_FEATURE_SVE
>> EOF
>         "MESON_GET_DEFINE_DELIMITER"
> 
> 

You should add sve in '-march' parameter, the same gcc version's output:

 ./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
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

./aarch64-linux-gnu-gcc -march=armv8.2-a+sve -dM -E - </dev/null 2>&1 | grep SVE
#define __ARM_FEATURE_SVE 1
#define __ARM_FEATURE_SVE_BITS 0

./aarch64-linux-gnu-gcc -march=armv8-a+sve -dM -E - </dev/null 2>&1 | grep SVE
#define __ARM_FEATURE_SVE 1
#define __ARM_FEATURE_SVE_BITS 0
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..8563d70 100644
--- a/drivers/net/hns3/meson.build
+++ b/drivers/net/hns3/meson.build
@@ -35,7 +35,20 @@  deps += ['hash']
 
 if arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
     sources += files('hns3_rxtx_vec.c')
+
+    # 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) != ''
+        cflags += ['-DCC_SVE_SUPPORT']
         sources += files('hns3_rxtx_vec_sve.c')
+    elif cc.has_argument('-march=armv8.2-a+sve')
+        cflags += ['-DCC_SVE_SUPPORT']
+        hns3_sve_lib = static_library('hns3_sve_lib',
+                        'hns3_rxtx_vec_sve.c',
+                        dependencies: [static_rte_ethdev],
+                        include_directories: includes,
+                        c_args: [cflags, '-march=armv8.2-a+sve'])
+        objs += hns3_sve_lib.extract_objects('hns3_rxtx_vec_sve.c')
     endif
 endif