| Message ID | 20241001111802.2728765-1-bruce.richardson@intel.com (mailing list archive) |
|---|---|
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4C15C45A7B; Tue, 1 Oct 2024 13:18:12 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1C967402CB; Tue, 1 Oct 2024 13:18:12 +0200 (CEST) Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.14]) by mails.dpdk.org (Postfix) with ESMTP id 56E654027E for <dev@dpdk.org>; Tue, 1 Oct 2024 13:18:10 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1727781490; x=1759317490; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=Yn1ilrQAkbyPdHV0KcrqRqf7JWlG2Uj/AhsAvjY2MMg=; b=aLgSEg/FNLv1ZHoPwtANHJ3FgSDuJ07ubEFUqj7ZcrFeMAiAxzLNpT0f mHaisFOujMTHl98vDH1WGmV3vWOq55gLsDrfN5dkk4sJUE7oR5RikSjGC NgQXbi93v3cwjV159R4fFHVRSjRTzQ42ydBaWC84EMJuwFIfqkSMhArAS AHQ5LU0Jj43iFXwrzrQv0/TQ8JANCNGk0z8NCWPvYvJs2dSnhkanNyUzo pg77PvRHSn3CIzftYt5mQBSMU0DBjfOPBedLC95pPlPgZ2s397rMZOA2s fDOlcln4S6PQXvhFcc9YtTV6JnuYcmQ0OaE2fywoxZ1LYy2vwyj4p/Nrl w==; X-CSE-ConnectionGUID: o/qf2reGREKvwB5e9TWGnQ== X-CSE-MsgGUID: RkUdcEOpTt6rG5r9BOOy/g== X-IronPort-AV: E=McAfee;i="6700,10204,11211"; a="27084531" X-IronPort-AV: E=Sophos;i="6.11,167,1725346800"; d="scan'208";a="27084531" Received: from orviesa007.jf.intel.com ([10.64.159.147]) by fmvoesa108.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Oct 2024 04:18:09 -0700 X-CSE-ConnectionGUID: tfdqmC02SfqEL+SYkKt9BA== X-CSE-MsgGUID: t6LPYzgeRpOMKuWv1ZDLnQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.11,167,1725346800"; d="scan'208";a="74047069" Received: from unknown (HELO silpixa00401385.ir.intel.com) ([10.237.214.25]) by orviesa007.jf.intel.com with ESMTP; 01 Oct 2024 04:18:09 -0700 From: Bruce Richardson <bruce.richardson@intel.com> To: dev@dpdk.org Cc: Bruce Richardson <bruce.richardson@intel.com> Subject: [PATCH v2 0/8] centralize AVX-512 feature detection Date: Tue, 1 Oct 2024 12:17:53 +0100 Message-ID: <20241001111802.2728765-1-bruce.richardson@intel.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240930175033.2283861-1-bruce.richardson@intel.com> References: <20240930175033.2283861-1-bruce.richardson@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org |
| Series |
centralize AVX-512 feature detection
|
|
Message
Bruce Richardson
Oct. 1, 2024, 11:17 a.m. UTC
The meson code to detect CPU and compiler support for AVX512 was duplicated across multiple drivers. Do all detection in just a single place to simplify the code. v2: ensure that target_has_avx512 is always defined on x86 to fix build errors Bruce Richardson (8): config/x86: add global defines for checking AVX-512 event/dlb2: use global AVX-512 variables common/idpf: use global AVX-512 variables net/cpfl: use global AVX-512 variables net/i40e: use global AVX-512 variables net/iavf: use global AVX-512 variables net/ice: use global AVX-512 variables net/idpf: use global AVX-512 variables config/x86/meson.build | 19 +++++++++++---- drivers/common/idpf/meson.build | 17 ++----------- drivers/event/dlb2/meson.build | 42 +++++++-------------------------- drivers/net/cpfl/meson.build | 19 ++------------- drivers/net/i40e/meson.build | 13 ++-------- drivers/net/iavf/meson.build | 13 ++-------- drivers/net/ice/meson.build | 15 ++---------- drivers/net/idpf/meson.build | 19 ++------------- 8 files changed, 36 insertions(+), 121 deletions(-) -- 2.43.0
Comments
On Tue, Oct 1, 2024 at 1:19 PM Bruce Richardson <bruce.richardson@intel.com> wrote: > > The meson code to detect CPU and compiler support for AVX512 was duplicated > across multiple drivers. Do all detection in just a single place to simplify > the code. > > v2: ensure that target_has_avx512 is always defined on x86 to fix build errors > > Bruce Richardson (8): > config/x86: add global defines for checking AVX-512 > event/dlb2: use global AVX-512 variables > common/idpf: use global AVX-512 variables > net/cpfl: use global AVX-512 variables > net/i40e: use global AVX-512 variables > net/iavf: use global AVX-512 variables > net/ice: use global AVX-512 variables > net/idpf: use global AVX-512 variables > > config/x86/meson.build | 19 +++++++++++---- > drivers/common/idpf/meson.build | 17 ++----------- > drivers/event/dlb2/meson.build | 42 +++++++-------------------------- > drivers/net/cpfl/meson.build | 19 ++------------- > drivers/net/i40e/meson.build | 13 ++-------- > drivers/net/iavf/meson.build | 13 ++-------- > drivers/net/ice/meson.build | 15 ++---------- > drivers/net/idpf/meson.build | 19 ++------------- > 8 files changed, 36 insertions(+), 121 deletions(-) Thanks for this cleanup, I have two comments. - Some drivers were going into great lenghts to check that individiual avx512 features were available. With this series, we end up requiring support for all features to announce avx512 availability. Are we perhaps disabling AVX512 support with some toolchains, out there, supporting only part of the set? - Some drivers were checking for presence of -mno-avx512f in machine_args as a way to disable building any AVX512 stuff. This gets discarded with this series.
On Tue, Oct 08, 2024 at 10:49:39AM +0200, David Marchand wrote: > On Tue, Oct 1, 2024 at 1:19 PM Bruce Richardson > <bruce.richardson@intel.com> wrote: > > > > The meson code to detect CPU and compiler support for AVX512 was duplicated > > across multiple drivers. Do all detection in just a single place to simplify > > the code. > > > > v2: ensure that target_has_avx512 is always defined on x86 to fix build errors > > > > Bruce Richardson (8): > > config/x86: add global defines for checking AVX-512 > > event/dlb2: use global AVX-512 variables > > common/idpf: use global AVX-512 variables > > net/cpfl: use global AVX-512 variables > > net/i40e: use global AVX-512 variables > > net/iavf: use global AVX-512 variables > > net/ice: use global AVX-512 variables > > net/idpf: use global AVX-512 variables > > > > config/x86/meson.build | 19 +++++++++++---- > > drivers/common/idpf/meson.build | 17 ++----------- > > drivers/event/dlb2/meson.build | 42 +++++++-------------------------- > > drivers/net/cpfl/meson.build | 19 ++------------- > > drivers/net/i40e/meson.build | 13 ++-------- > > drivers/net/iavf/meson.build | 13 ++-------- > > drivers/net/ice/meson.build | 15 ++---------- > > drivers/net/idpf/meson.build | 19 ++------------- > > 8 files changed, 36 insertions(+), 121 deletions(-) > > Thanks for this cleanup, I have two comments. > > - Some drivers were going into great lenghts to check that individiual > avx512 features were available. > With this series, we end up requiring support for all features to > announce avx512 availability. > Are we perhaps disabling AVX512 support with some toolchains, out > there, supporting only part of the set? > The various AVX-512 feature sets checked for (F, BW, VL, DQ) were all introduced in the same hardware generation - all are available in gcc when using -march=skylake-avx512 or later, or -march=znver4. On the toolchain side, gcc introduced all these flags simultaneously in gcc-6 [1]. For clang/llvm, testing with godbolt for compiler errors/warnings indicates that all these 4 avx512 flags are available from clang 3.6 - the minimum we support in DPDK [2] [1] https://gcc.gnu.org/gcc-6/changes.html [2] https://doc.dpdk.org/guides/linux_gsg/sys_reqs.html#compilation-of-the-dpdk > - Some drivers were checking for presence of -mno-avx512f in > machine_args as a way to disable building any AVX512 stuff. > This gets discarded with this series. > Yes, because it should no longer be necessary. The places in the build system where we set the no-avx512f flag are reworked so that we don't have cc_has_avx512 set. Regards, /Bruce
On Tue, Oct 8, 2024 at 12:03 PM Bruce Richardson <bruce.richardson@intel.com> wrote: > > On Tue, Oct 08, 2024 at 10:49:39AM +0200, David Marchand wrote: > > On Tue, Oct 1, 2024 at 1:19 PM Bruce Richardson > > <bruce.richardson@intel.com> wrote: > > > > > > The meson code to detect CPU and compiler support for AVX512 was duplicated > > > across multiple drivers. Do all detection in just a single place to simplify > > > the code. > > > > > > v2: ensure that target_has_avx512 is always defined on x86 to fix build errors > > > > > > Bruce Richardson (8): > > > config/x86: add global defines for checking AVX-512 > > > event/dlb2: use global AVX-512 variables > > > common/idpf: use global AVX-512 variables > > > net/cpfl: use global AVX-512 variables > > > net/i40e: use global AVX-512 variables > > > net/iavf: use global AVX-512 variables > > > net/ice: use global AVX-512 variables > > > net/idpf: use global AVX-512 variables > > > > > > config/x86/meson.build | 19 +++++++++++---- > > > drivers/common/idpf/meson.build | 17 ++----------- > > > drivers/event/dlb2/meson.build | 42 +++++++-------------------------- > > > drivers/net/cpfl/meson.build | 19 ++------------- > > > drivers/net/i40e/meson.build | 13 ++-------- > > > drivers/net/iavf/meson.build | 13 ++-------- > > > drivers/net/ice/meson.build | 15 ++---------- > > > drivers/net/idpf/meson.build | 19 ++------------- > > > 8 files changed, 36 insertions(+), 121 deletions(-) > > > > Thanks for this cleanup, I have two comments. > > > > - Some drivers were going into great lenghts to check that individiual > > avx512 features were available. > > With this series, we end up requiring support for all features to > > announce avx512 availability. > > Are we perhaps disabling AVX512 support with some toolchains, out > > there, supporting only part of the set? > > > > The various AVX-512 feature sets checked for (F, BW, VL, DQ) were all > introduced in the same hardware generation - all are available in gcc when > using -march=skylake-avx512 or later, or -march=znver4. On the toolchain > side, gcc introduced all these flags simultaneously in gcc-6 [1]. For > clang/llvm, testing with godbolt for compiler errors/warnings indicates > that all these 4 avx512 flags are available from clang 3.6 - the minimum we > support in DPDK [2] > > [1] https://gcc.gnu.org/gcc-6/changes.html > [2] https://doc.dpdk.org/guides/linux_gsg/sys_reqs.html#compilation-of-the-dpdk Perfect, thanks for the details. > > > - Some drivers were checking for presence of -mno-avx512f in > > machine_args as a way to disable building any AVX512 stuff. > > This gets discarded with this series. > > > > Yes, because it should no longer be necessary. The places in the build > system where we set the no-avx512f flag are reworked so that we don't have > cc_has_avx512 set. Ok, it is clearer now. Last comment on style: $ git grep cc_avx512_flags drivers/ drivers/common/idpf/meson.build: avx512_args = [cflags] + cc_avx512_flags drivers/event/dlb2/meson.build: c_args: cflags + cc_avx512_flags) drivers/net/i40e/meson.build: avx512_args = cflags + cc_avx512_flags drivers/net/iavf/meson.build: avx512_args = cflags + cc_avx512_flags drivers/net/ice/meson.build: avx512_args = cflags + cc_avx512_flags I think it is safe to remove the [] around cflags in common/idpf, right? Do you have some cycles to send a v2 and convert lib/net and net/virtio ? Otherwise, can you do a followup patch for rc2? Thanks.
On Tue, Oct 08, 2024 at 01:33:16PM +0200, David Marchand wrote: > On Tue, Oct 8, 2024 at 12:03 PM Bruce Richardson > <bruce.richardson@intel.com> wrote: > > > > On Tue, Oct 08, 2024 at 10:49:39AM +0200, David Marchand wrote: > > > On Tue, Oct 1, 2024 at 1:19 PM Bruce Richardson > > > <bruce.richardson@intel.com> wrote: > > > > > > > > The meson code to detect CPU and compiler support for AVX512 was duplicated > > > > across multiple drivers. Do all detection in just a single place to simplify > > > > the code. > > > > > > > > v2: ensure that target_has_avx512 is always defined on x86 to fix build errors > > > > > > > > Bruce Richardson (8): > > > > config/x86: add global defines for checking AVX-512 > > > > event/dlb2: use global AVX-512 variables > > > > common/idpf: use global AVX-512 variables > > > > net/cpfl: use global AVX-512 variables > > > > net/i40e: use global AVX-512 variables > > > > net/iavf: use global AVX-512 variables > > > > net/ice: use global AVX-512 variables > > > > net/idpf: use global AVX-512 variables > > > > > > > > config/x86/meson.build | 19 +++++++++++---- > > > > drivers/common/idpf/meson.build | 17 ++----------- > > > > drivers/event/dlb2/meson.build | 42 +++++++-------------------------- > > > > drivers/net/cpfl/meson.build | 19 ++------------- > > > > drivers/net/i40e/meson.build | 13 ++-------- > > > > drivers/net/iavf/meson.build | 13 ++-------- > > > > drivers/net/ice/meson.build | 15 ++---------- > > > > drivers/net/idpf/meson.build | 19 ++------------- > > > > 8 files changed, 36 insertions(+), 121 deletions(-) > > > > > > Thanks for this cleanup, I have two comments. > > > > > > - Some drivers were going into great lenghts to check that individiual > > > avx512 features were available. > > > With this series, we end up requiring support for all features to > > > announce avx512 availability. > > > Are we perhaps disabling AVX512 support with some toolchains, out > > > there, supporting only part of the set? > > > > > > > The various AVX-512 feature sets checked for (F, BW, VL, DQ) were all > > introduced in the same hardware generation - all are available in gcc when > > using -march=skylake-avx512 or later, or -march=znver4. On the toolchain > > side, gcc introduced all these flags simultaneously in gcc-6 [1]. For > > clang/llvm, testing with godbolt for compiler errors/warnings indicates > > that all these 4 avx512 flags are available from clang 3.6 - the minimum we > > support in DPDK [2] > > > > [1] https://gcc.gnu.org/gcc-6/changes.html > > [2] https://doc.dpdk.org/guides/linux_gsg/sys_reqs.html#compilation-of-the-dpdk > > Perfect, thanks for the details. > > > > > > - Some drivers were checking for presence of -mno-avx512f in > > > machine_args as a way to disable building any AVX512 stuff. > > > This gets discarded with this series. > > > > > > > Yes, because it should no longer be necessary. The places in the build > > system where we set the no-avx512f flag are reworked so that we don't have > > cc_has_avx512 set. > > Ok, it is clearer now. > > Last comment on style: > $ git grep cc_avx512_flags drivers/ > drivers/common/idpf/meson.build: avx512_args = [cflags] + cc_avx512_flags > drivers/event/dlb2/meson.build: c_args: > cflags + cc_avx512_flags) > drivers/net/i40e/meson.build: avx512_args = cflags + cc_avx512_flags > drivers/net/iavf/meson.build: avx512_args = cflags + cc_avx512_flags > drivers/net/ice/meson.build: avx512_args = cflags + cc_avx512_flags > > I think it is safe to remove the [] around cflags in common/idpf, right? > Yep. > > Do you have some cycles to send a v2 and convert lib/net and net/virtio ? > Otherwise, can you do a followup patch for rc2? > I'll see what I can do today. /Bruce