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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

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