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 07B2F428D4; Wed, 5 Apr 2023 17:51:59 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0B93442BB1; Wed, 5 Apr 2023 17:51:56 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 3F35741153 for ; Wed, 5 Apr 2023 17:51:53 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 8D019210DED5; Wed, 5 Apr 2023 08:51:52 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 8D019210DED5 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1680709912; bh=lkDU+94XQtYvnae6oh3hW9SofgRzyZsyZCYatloqJys=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=dKd9Ar2Ne8i4RL/5/IdP5aYTjUwnpZ4245Rzi78lx60yC/EjmxyEfNF50GMS+7vlf W9jInHzZqYWNyRQVC8xeIN5NepaYRsIfp4ADcV/Yr2DF8lbSAC2mhEpiBj06iYt19n 6+9DICXWkhKC306x/CXbNygT7t5nwRhFJ1f9iU80= Date: Wed, 5 Apr 2023 08:51:52 -0700 From: Tyler Retzlaff To: Bruce Richardson Cc: dev@dpdk.org, david.marchand@redhat.com, thomas@monjalon.net, mb@smartsharesystems.com, konstantin.ananyev@huawei.com Subject: Re: [PATCH v2 6/9] eal: expand most macros to empty when using msvc Message-ID: <20230405155152.GC31673@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1680558751-17931-1-git-send-email-roretzla@linux.microsoft.com> <1680638847-26430-1-git-send-email-roretzla@linux.microsoft.com> <1680638847-26430-7-git-send-email-roretzla@linux.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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 On Wed, Apr 05, 2023 at 11:44:43AM +0100, Bruce Richardson wrote: > On Tue, Apr 04, 2023 at 01:07:24PM -0700, Tyler Retzlaff wrote: > > 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 | 33 +++++++++++++++++++++++++++++++++ > > lib/eal/include/rte_compat.h | 20 ++++++++++++++++++++ > > 3 files changed, 61 insertions(+) > > > > diff --git a/lib/eal/include/rte_branch_prediction.h b/lib/eal/include/rte_branch_prediction.h > > index 0256a9d..3589c97 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) == 1) > > +#endif > > I think this should just be "#define likely(x) (x)", since the likely is > just a hint as to which way we expect the branch to go. It does not change > the logic in the expression. > > > #endif /* likely */ > > > > /** > > @@ -39,7 +43,11 @@ > > * > > */ > > #ifndef unlikely > > +#ifndef RTE_TOOLCHAIN_MSVC > > #define unlikely(x) __builtin_expect(!!(x), 0) > > +#else > > +#define unlikely(x) (!!(x) == 0) > > +#endif > > This expansion is wrong, because it changes the logic of the expression, > rather than being a hint. As above with likely, I think this should just > expand as "(x)", making the unlikely ignored. ooh, obviously wrong will fix. > > > #endif /* unlikely */ > > > > #ifdef __cplusplus > > diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h > > index 2f464e3..a724e22 100644 > > --- a/lib/eal/include/rte_common.h > > +++ b/lib/eal/include/rte_common.h > > This file has a lot of ifdefs in it now for msvc. Couple of suggestions: > > 1. can we group these defines together so we can hit multiple entries with a > single msvc block? > > 2. alternatively, for those we want to just permanently null, we could > split the responsibility for them between the currnet headers and possibly > the build system. Specifically, rather than doing macros based on MSVC, > change each macro to the simpler: > > #ifndef MACRO > #define MACRO macro_definition > #endif > > Then in the meson.build processing, we can have some separte MSVC > processing to put null defines for these into rte_build_config.h, or put > in null defines for them in some MSVC-specific header. yes, i'll do this for now. i won't go the route of rte_build_config.h for now since some of the macros i will eventually try to find a way to provide equivalent expansion. > > Just some thoughts. > /Bruce