From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4858EA0A02; Tue, 18 May 2021 18:37:23 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 069F14068E; Tue, 18 May 2021 18:37:23 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mails.dpdk.org (Postfix) with ESMTP id 79C3D40041 for ; Tue, 18 May 2021 18:37:20 +0200 (CEST) IronPort-SDR: ACtjwgNp78lyoudwDKOWe8+ElNqHmwFoL8M8Tc4YRJn/PGUU4YGiqzpk2AapZoVdLP3UPgQ2xJ +mPpLs0h8iEA== X-IronPort-AV: E=McAfee;i="6200,9189,9988"; a="188168141" X-IronPort-AV: E=Sophos;i="5.82,310,1613462400"; d="scan'208";a="188168141" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 May 2021 09:37:19 -0700 IronPort-SDR: hkcMBVoJ+zacntvUFuKbvgw/LopIpNbjkjHPi+zGh2QA3iJu2noT5MspNlCwDiLNKydlpXYF5y XcHqxOGbydDg== X-IronPort-AV: E=Sophos;i="5.82,310,1613462400"; d="scan'208";a="439517458" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.224.73]) ([10.213.224.73]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 May 2021 09:37:16 -0700 To: Honnappa Nagarahalli , Chengwen Feng , "thomas@monjalon.net" , Bruce Richardson Cc: "dev@dpdk.org" , "jerinj@marvell.com" , Ruifeng Wang , "viktorin@rehivetech.com" , "jerinjacobk@gmail.com" , "juraj.linkes@pantheon.tech" , nd References: <1620808126-18876-1-git-send-email-fengchengwen@huawei.com> <1620986039-29475-1-git-send-email-fengchengwen@huawei.com> <1620986039-29475-3-git-send-email-fengchengwen@huawei.com> <3028dea0-97f6-ed06-8017-418fd55e72a3@intel.com> <3aaf306b-79c6-42e7-d00c-07fac13ac640@intel.com> From: Ferruh Yigit X-User: ferruhy Message-ID: Date: Tue, 18 May 2021 17:37:13 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v5 2/2] net/hns3: refactor SVE code compile method X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 5/18/2021 5:12 PM, Honnappa Nagarahalli wrote: >>> >>>> >>>> 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. >> >>> 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 >>>>> --- >>>>> 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 >>>>> >>> >