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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 5/20/2021 2:11 AM, fengchengwen wrote:
> 
> 
> On 2021/5/19 23:02, Ferruh Yigit wrote:
>> On 5/19/2021 2:25 PM, 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
>>>
>>> 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>
>>
>> The patch passes the CI build [1], but in that test sve file is not compiled at
>> all because of missing header file [2].
>>
> 
> Yes, it was designed as it.
> In hns3 meson.build we call cc.check_header('arm_sve.h'), and gcc9 don't have this headerfile.
> 
>> I wonder if the warning caused by conflicting switch [3] is still valid, we need
>> a platform that sve file is compiled to verify this. Do you have this
>> environment, that sets '-mcpu=armv8.1-a'.
>>
> 
> It already fix by filterout '-march' '-mcpu' '-mtune' in hns3 meson.build of this patch
> If don't add the above filtout logic:
>   a) Test result with gcc8.3 and crossfile thunder2:
>     cc1: warning: switch ‘-mcpu=thunderx2t99’ conflicts with ‘-march=armv8.2-a+sve’ switch
>   b) Test result with gcc9.2 and crossfile thunder2:
>     cc1: warning: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’ switch
> 
> Note: the gcc8.3/9.2 version detail:
>   ./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
>   ./aarch64-none-linux-gnu-gcc --version
>   aarch64-none-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 9.2-2019.12 (arm-9.10)) 9.2.1 20191025
> 

Hi Chengwen,

-rc4 is already out and release is planned for tomorrow, so it is late to get
this patch now, let's proceed with it in next release.

>>
>> Btw, CI reports unit test failure, I don't think it is related with this patch
>> but can you please double check?
>>
> 
> Agree, it is atomic_autotest and malloc_autotest failed, it hasn't run any hns3 driver's code.
> 
>>
>>
>> [1]
>> https://lab.dpdk.org/results/dashboard/patchsets/17135/
>>
>> [2]
>> Check usable header "arm_sve.h" : NO
>>
>> [3]
>> error: switch ‘-mcpu=armv8.1-a’ conflicts with ‘-march=armv8.2-a’ switch [-Werror]
>>
>>> ---
>>>  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']
>>>          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
>>>
>>
>>
>> .
>>
>