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