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 5182642941; Fri, 14 Apr 2023 14:39:07 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E30D040144; Fri, 14 Apr 2023 14:39:06 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 52AC0400D5 for ; Fri, 14 Apr 2023 14:39:05 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v5 11/14] eal: expand most macros to empty when using MSVC Date: Fri, 14 Apr 2023 14:39:03 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87879@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v5 11/14] eal: expand most macros to empty when using MSVC Thread-Index: AdlusqtIFNybBNTXR1eYPRo8dZHolAAF4XPg References: <1680558751-17931-1-git-send-email-roretzla@linux.microsoft.com> <1681421163-18578-1-git-send-email-roretzla@linux.microsoft.com> <1681421163-18578-12-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35D87878@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" , "Tyler Retzlaff" Cc: , , , 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 > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > Sent: Friday, 14 April 2023 11.22 >=20 > On Fri, Apr 14, 2023 at 08:45:17AM +0200, Morten Br=F8rup wrote: > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > > Sent: Thursday, 13 April 2023 23.26 > > > > > > For now expand a lot of common rte macros empty. The catch here is = we > > > need to test that most of the macros do what they should but at = the same > > > time they are blocking work needed to bootstrap of the unit tests. > > > > > > Later we will return and provide (where possible) expansions that = work > > > correctly for msvc and where not possible provide some alternate = macros > > > to achieve the same outcome. > > > > > > Signed-off-by: Tyler Retzlaff > > > --- > > > lib/eal/include/rte_branch_prediction.h | 8 ++++++ > > > lib/eal/include/rte_common.h | 45 > > > +++++++++++++++++++++++++++++++++ > > > lib/eal/include/rte_compat.h | 20 +++++++++++++++ > > > 3 files changed, 73 insertions(+) > > > > > > diff --git a/lib/eal/include/rte_branch_prediction.h > > > b/lib/eal/include/rte_branch_prediction.h > > > index 0256a9d..d9a0224 100644 > > > --- a/lib/eal/include/rte_branch_prediction.h > > > +++ b/lib/eal/include/rte_branch_prediction.h > > > @@ -25,7 +25,11 @@ > > > * > > > */ > > > #ifndef likely > > > +#ifndef RTE_TOOLCHAIN_MSVC > > > #define likely(x) __builtin_expect(!!(x), 1) > > > +#else > > > +#define likely(x) (x) > > > > This must be (!!(x)), because x may be non-Boolean, e.g. likely(n & = 0x10), > and likely() must return Boolean (0 or 1). > > >=20 > Will this really make a difference? Is there somewhere likely/unlikely > would be used where we would not get the same conversion to boolean = than we > get using "!!" operator. [NOTE: Not saying we shouldn't put in the !!, = just > wondering if there are actual cases where it affects the output?] I agree that it makes no difference the way it is typically used. But there are creative developers out there, so these macros definitely = need the "!!" conversion to Boolean. >=20 > > > +#endif > > > #endif /* likely */ > > > > > > /** > > > @@ -39,7 +43,11 @@ > > > * > > > */ > > > #ifndef unlikely > > > +#ifndef RTE_TOOLCHAIN_MSVC > > > #define unlikely(x) __builtin_expect(!!(x), 0) > > > +#else > > > +#define unlikely(x) (x) > > > > This must also be (!!(x)), for the same reason as above. > > > > > +#endif > > > #endif /* unlikely */ > > > > > > #ifdef __cplusplus > > > diff --git a/lib/eal/include/rte_common.h = b/lib/eal/include/rte_common.h > > > index 2f464e3..1bdaa2d 100644 > > > --- a/lib/eal/include/rte_common.h > > > +++ b/lib/eal/include/rte_common.h > > > @@ -65,7 +65,11 @@ > > > /** > > > * Force alignment > > > */ > > > +#ifndef RTE_TOOLCHAIN_MSVC > > > #define __rte_aligned(a) __attribute__((__aligned__(a))) > > > +#else > > > +#define __rte_aligned(a) > > > +#endif > > > > It should be reviewed that __rte_aligned() is only used for = optimization > purposes, and is not required for DPDK to function properly. > > >=20 > Good point. >=20 > If we look across all of DPDK, things will likely break, as we are = relying > on alignment in various places to use the aligned versions of = instructions. > For example _mm256_load_si256() vs _mm256_loadu_si256() in our x86 > vectorized driver code. A "git grep _load_si" shows quite a few = aligned > vector load instructions used in our codebase. These will fault and = cause a > crash if the data is not properly aligned. [I suspect that there are = similar > restrictions on other architectures too, just not familiar with their > intrinsics to check.] Another thing that has been annoying me with the use of vector = instructions: Vector instructions are often used in a way where they cast away the = type they are working on, so if that type is modified (e.g. a field is = moved), the code will happily build, but fail at runtime. When casting away the type for vector instructions, _Static_assert or = BUILD_BUG_ON should be used to verify the assumptions about the cast = away type. Such a practice might catch some of the places where the = missing alignment (and missing structure packing) would fail. >=20 > However, it may be that none of the code paths where these are used is > in code currently compiled on windows, so this may be safe for now. = The > occurances are mostly in drivers. >=20 > $ git grep -l _load_si > drivers/common/idpf/idpf_common_rxtx_avx512.c > drivers/event/dlb2/dlb2.c > drivers/net/bnxt/bnxt_rxtx_vec_avx2.c > drivers/net/bnxt/bnxt_rxtx_vec_sse.c > drivers/net/enic/enic_rxtx_vec_avx2.c > drivers/net/i40e/i40e_rxtx_vec_avx2.c > drivers/net/i40e/i40e_rxtx_vec_avx512.c > drivers/net/iavf/iavf_rxtx_vec_avx2.c > drivers/net/iavf/iavf_rxtx_vec_avx512.c > drivers/net/iavf/iavf_rxtx_vec_sse.c > drivers/net/ice/ice_rxtx_vec_avx2.c > drivers/net/ice/ice_rxtx_vec_avx512.c > drivers/net/ice/ice_rxtx_vec_sse.c > drivers/net/mlx5/mlx5_rxtx_vec_sse.h > lib/acl/acl_bld.c > lib/distributor/rte_distributor_match_sse.c > lib/efd/rte_efd_x86.h > lib/hash/rte_cuckoo_hash.c > lib/member/rte_member_x86.h > lib/net/net_crc_avx512.c > lib/net/net_crc_sse.c >=20 >=20 > > > > > > #ifdef RTE_ARCH_STRICT_ALIGN > > > typedef uint64_t unaligned_uint64_t __rte_aligned(1); > > > @@ -80,16 +84,29 @@ > > > /** > > > * Force a structure to be packed > > > */ > > > +#ifndef RTE_TOOLCHAIN_MSVC > > > #define __rte_packed __attribute__((__packed__)) > > > +#else > > > +#define __rte_packed > > > +#endif > > > > Similar comment as for __rte_aligned(); however, I consider it more = likely > that structure packing is a functional requirement, and not just used = for > optimization. Based on my experience, it may be used for packing = network > structures; perhaps not in DPDK itself but maybe in DPDK applications. > > >=20 > +1 > Once libraries such as the net library in DPDK will form part of the > windows build this will need to be addressed or things will break. Yes. And for application developers, we should deprecate and replace the = __rte_packed macro with something that works on both MSVC and GCC/CLANG. = The same probably goes for __rte_aligned(). But, let's not hold back Tyler's work. Just put it on the long term TODO = list for MSVC support. >=20 > > The same risk applies to __rte_aligned(), but with lower = probability. > > >=20 > /Bruce