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?] > > +#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.] 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. $ 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 > > > > #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. > +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. > The same risk applies to __rte_aligned(), but with lower probability. > /Bruce
