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

[fengchengwen <fengchengwen@huawei.com>]

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