On Fri, Apr 14, 2023 at 02:39:03PM +0200, Morten Brørup wrote: > > From: Bruce Richardson [mailto:bruce.richard...@intel.com] > > Sent: Friday, 14 April 2023 11.22 > > > > On Fri, Apr 14, 2023 at 08:45:17AM +0200, Morten Brørup wrote: > > > > From: Tyler Retzlaff [mailto:roret...@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 <roret...@linux.microsoft.com> > > > > --- > > > > 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). > > > > > > > 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. >
Sure. > > > > > > +#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. > > > > > > > Good point. > > > > 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. > Agreed. And, in fairness, this is sometimes done in our code, e.g. [1], but should probably be more widely done. It's something we should try and catch in reviews of vector code, as it also helps document what exactly we are doing and why. /Bruce [1] http://git.dpdk.org/dpdk/tree/drivers/net/i40e/i40e_rxtx_vec_avx2.c#n183
