On Wed, Jan 27, 2021 at 9:06 AM Ruifeng Wang <ruifeng.w...@arm.com> wrote:
>
> > -----Original Message-----
> > From: Joyce Kong <joyce.k...@arm.com>
> > Sent: Friday, January 15, 2021 5:58 PM
> > To: jer...@marvell.com; david.march...@redhat.com; Ruifeng Wang
> > <ruifeng.w...@arm.com>; Honnappa Nagarahalli
> > <honnappa.nagaraha...@arm.com>
> > Cc: dev@dpdk.org; nd <n...@arm.com>; sta...@dpdk.org
> > Subject: [PATCH v1] eal/arm: fix gcc build for 128-bit atomic compare
> > exchange
> >
> > Compiling with "meson build -Dbuildtype=debug --cross-file
> > config/arm/arm64_thunderx2_linux_gcc" shows the warnings "function
>
> Issue can be reproduced with the posted command. But it is not specific to
> ThunderX2 platform.
> It is reproducible on any platform that has LSE extension when building with
> 'buildtype=debug'.
>
> Reviewed-by: Ruifeng Wang <ruifeng.w...@arm.com>
Acked-by: Jerin Jacob <jer...@marvell.com>
>
> > returns an aggregate [-Waggregate-return]":
> > ../../dpdk/lib/librte_eal/arm/include/rte_atomic_64.h: In function
> > ‘__cas_128_relaxed’:
> > ../../dpdk/lib/librte_eal/arm/include/rte_atomic_64.h:81:20:
> > error: function returns an aggregate [-Werror=aggregate-return]
> > __ATOMIC128_CAS_OP(__cas_128_relaxed, "casp")
> > ^~~~~~~~~~~~~~~~~
> >
> > Fix the compiling issue by defining __ATOMIC128_CAS_OP as a void function
> > and passing the address pointer into it.
> >
> > Fixes: 7e2c3e17fe2c ("eal/arm64: add 128-bit atomic compare exchange")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Joyce Kong <joyce.k...@arm.com>
> > ---
> > lib/librte_eal/arm/include/rte_atomic_64.h | 28 +++++++++++-----------
> > 1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/lib/librte_eal/arm/include/rte_atomic_64.h
> > b/lib/librte_eal/arm/include/rte_atomic_64.h
> > index 467d32a45..fa6f334c0 100644
> > --- a/lib/librte_eal/arm/include/rte_atomic_64.h
> > +++ b/lib/librte_eal/arm/include/rte_atomic_64.h
> > @@ -53,15 +53,15 @@ rte_atomic_thread_fence(int memorder) #endif
> >
> > #define __ATOMIC128_CAS_OP(cas_op_name, op_string)
> > \
> > -static __rte_noinline rte_int128_t
> > \
> > -cas_op_name(rte_int128_t *dst, rte_int128_t old, rte_int128_t updated)
> > \
> > +static __rte_noinline void
> > \
> > +cas_op_name(rte_int128_t *dst, rte_int128_t *old, rte_int128_t updated)
> > \
> > {
> > \
> > /* caspX instructions register pair must start from even-numbered
> > * register at operand 1.
> > * So, specify registers for local variables here.
> > */ \
> > - register uint64_t x0 __asm("x0") = (uint64_t)old.val[0]; \
> > - register uint64_t x1 __asm("x1") = (uint64_t)old.val[1]; \
> > + register uint64_t x0 __asm("x0") = (uint64_t)old->val[0]; \
> > + register uint64_t x1 __asm("x1") = (uint64_t)old->val[1]; \
> > register uint64_t x2 __asm("x2") = (uint64_t)updated.val[0]; \
> > register uint64_t x3 __asm("x3") = (uint64_t)updated.val[1]; \
> > asm volatile( \
> > @@ -73,9 +73,8 @@ cas_op_name(rte_int128_t *dst, rte_int128_t old,
> > rte_int128_t updated) \
> > [upd1] "r" (x3), \
> > [dst] "r" (dst) \
> > : "memory"); \
> > - old.val[0] = x0; \
> > - old.val[1] = x1; \
> > - return old; \
> > + old->val[0] = x0; \
> > + old->val[1] = x1; \
> > }
> >
> > __ATOMIC128_CAS_OP(__cas_128_relaxed, "casp") @@ -113,13 +112,14
> > @@ rte_atomic128_cmp_exchange(rte_int128_t *dst, rte_int128_t *exp,
> >
> > #if defined(__ARM_FEATURE_ATOMICS) ||
> > defined(RTE_ARM_FEATURE_ATOMICS)
> > if (success == __ATOMIC_RELAXED)
> > - old = __cas_128_relaxed(dst, expected, desired);
> > + __cas_128_relaxed(dst, exp, desired);
> > else if (success == __ATOMIC_ACQUIRE)
> > - old = __cas_128_acquire(dst, expected, desired);
> > + __cas_128_acquire(dst, exp, desired);
> > else if (success == __ATOMIC_RELEASE)
> > - old = __cas_128_release(dst, expected, desired);
> > + __cas_128_release(dst, exp, desired);
> > else
> > - old = __cas_128_acq_rel(dst, expected, desired);
> > + __cas_128_acq_rel(dst, exp, desired);
> > + old = *exp;
> > #else
> > #define __HAS_ACQ(mo) ((mo) != __ATOMIC_RELAXED && (mo) !=
> > __ATOMIC_RELEASE) #define __HAS_RLS(mo) ((mo) ==
> > __ATOMIC_RELEASE || (mo) == __ATOMIC_ACQ_REL || \ @@ -183,12
> > +183,12 @@ rte_atomic128_cmp_exchange(rte_int128_t *dst, rte_int128_t
> > *exp, #undef __STORE_128
> >
> > } while (unlikely(ret));
> > -#endif
> >
> > - /* Unconditionally updating expected removes an 'if' statement.
> > - * expected should already be in register if not in the cache.
> > + /* Unconditionally updating the value of exp removes an 'if'
> > statement.
> > + * The value of exp should already be in register if not in the cache.
> > */
> > *exp = old;
> > +#endif
> >
> > return (old.int128 == expected.int128); }
> > --
> > 2.30.0
>
