Re: [dpdk-dev] [PATCH v6 2/2] net/hns3: refactor SVE code compile method

[Ruifeng Wang <Ruifeng.Wang@arm.com>]

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