On 5/18/2021 4:48 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.
> 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: sta...@dpdk.org
>>>
>>> Signed-off-by: Chengwen Feng <fengcheng...@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
>>>
>
