On Wed, Nov 15, 2023 at 10:03 PM Tyler Retzlaff <roret...@linux.microsoft.com> wrote: > > On Wed, Nov 15, 2023 at 09:08:05PM +0100, Morten Brørup wrote: > > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > > > Sent: Wednesday, 15 November 2023 18.40 > > > > > > Now that we have enabled C11 replace the use of __rte_cache_aligned > > > and __rte_aligned(n) with alignas(RTE_CACHE_LINE_SIZE) and > > > __rte_aligned(n) respectively. > > > > > > > [...] > > > > > typedef union rte_xmm { > > > + alignas(16) > > > xmm_t x; > > > uint8_t u8[XMM_SIZE / sizeof(uint8_t)]; > > > uint16_t u16[XMM_SIZE / sizeof(uint16_t)]; > > > uint32_t u32[XMM_SIZE / sizeof(uint32_t)]; > > > uint64_t u64[XMM_SIZE / sizeof(uint64_t)]; > > > double pd[XMM_SIZE / sizeof(double)]; > > > -} __rte_aligned(16) rte_xmm_t; > > > +} rte_xmm_t; > > > > Your patch message should mention that C11 doesn't allow alignas() being > > applied to the declarations of struct/union types, so it is applied to the > > first field in the struct/union, which has the same effect. > > no problem, will add a note. > > > > > Someone unfamiliar with alignas() would expect: > > > > -typedef union rte_xmm { > > +typedef alignas(16) union rte_xmm { > > [...] > > -} __rte_aligned(16) rte_xmm_t; > > +} rte_xmm_t; > > > > [...] > > > > > #ifndef RTE_VECT_RISCV_H > > > #define RTE_VECT_RISCV_H > > > > > > +#include <stdalign.h> > > > #include <stdint.h> > > > #include "generic/rte_vect.h" > > > #include "rte_common.h" > > > @@ -23,13 +24,14 @@ > > > #define XMM_MASK (XMM_SIZE - 1) > > > > > > typedef union rte_xmm { > > > + alignas(16) /* !! NOTE !! changed to 16 it looks like this was a > > > bug? */ > > > xmm_t x; > > > uint8_t u8[XMM_SIZE / sizeof(uint8_t)]; > > > uint16_t u16[XMM_SIZE / sizeof(uint16_t)]; > > > uint32_t u32[XMM_SIZE / sizeof(uint32_t)]; > > > uint64_t u64[XMM_SIZE / sizeof(uint64_t)]; > > > double pd[XMM_SIZE / sizeof(double)]; > > > -} __rte_aligned(8) rte_xmm_t; > > > +} rte_xmm_t; > > > > Yes, this looks very much like a bug. > > Even if a RISC-V CPU could handle alignment like that, it might interact > > with other software/hardware expecting type-sized alignment, i.e. 16-byte > > alignment, so partially using 8-byte alignment would cause bugs. > > > > It should be a separate patch with a Fixes tag. > > i'll submit a patch/fix for this so it is available and others can > discuss if it should or shouldn't be merged for 23.11. It is definitely a bug. Good catch. Since we did not have vector extensions on our bring-up board, all xmm_t handling was essentially scalar. > > > > > We need to urgently decide if this bug should live on in DPDK 23.11, or if > > the fix should be included although we are very late in the release process. > > > > Stanislaw, what do you think? > > > > Furthermore, I wonder if it can be backported to stable, and to what extent > > backporting it would break the ABI/API. > >
-- Best Regards, Stanisław Kardach
