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