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