Re: [RFC PATCH v1 2/2] riscv/cmpxchg: Deduplicate xchg() asm functions
From: Guo Ren
Date: Fri Apr 07 2023 - 04:32:15 EST
On Thu, Apr 6, 2023 at 4:20 PM Leonardo Bras <leobras@xxxxxxxxxx> wrote:
>
> In this header every xchg define (_relaxed, _acquire, _release, vanilla)
> contain it's own asm file, both for 4-byte variables an 8-byte variables,
> on a total of 8 versions of mostly the same asm.
>
> This is usually bad, as it means any change may be done in up to 8
> different places.
>
> Unify those versions by creating a new define with enough parameters to
> generate any version of the previous 8.
>
> Then unify the result under a more general define, and simplify
> arch_xchg* generation.
>
> (This did not cause any change in generated asm)
>
> Signed-off-by: Leonardo Bras <leobras@xxxxxxxxxx>
> ---
> arch/riscv/include/asm/cmpxchg.h | 135 +++++++------------------------
> 1 file changed, 31 insertions(+), 104 deletions(-)
>
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index f88fae357071c..905a888d8b04d 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -11,25 +11,30 @@
> #include <asm/barrier.h>
> #include <asm/fence.h>
>
> -#define __xchg_relaxed(ptr, new, size) \
> +#define ____xchg(sfx, prepend, append, r, p, n) \
> +({ \
> + __asm__ __volatile__ ( \
> + prepend \
> + " amoswap" sfx " %0, %2, %1\n" \
> + append \
> + : "=r" (r), "+A" (*(p)) \
> + : "r" (n) \
> + : "memory"); \
> +})
> +
> +#define ___xchg(ptr, new, size, sfx, prepend, append) \
> ({ \
> __typeof__(ptr) __ptr = (ptr); \
> __typeof__(new) __new = (new); \
> __typeof__(*(ptr)) __ret; \
> switch (size) { \
> case 4: \
> - __asm__ __volatile__ ( \
> - " amoswap.w %0, %2, %1\n" \
> - : "=r" (__ret), "+A" (*__ptr) \
> - : "r" (__new) \
> - : "memory"); \
> + ____xchg(".w" sfx, prepend, append, \
> + __ret, __ptr, __new); \
> break; \
> case 8: \
> - __asm__ __volatile__ ( \
> - " amoswap.d %0, %2, %1\n" \
> - : "=r" (__ret), "+A" (*__ptr) \
> - : "r" (__new) \
> - : "memory"); \
> + ____xchg(".d" sfx, prepend, append, \
> + __ret, __ptr, __new); \
> break; \
> default: \
> BUILD_BUG(); \
> @@ -37,114 +42,36 @@
> __ret; \
> })
>
> -#define arch_xchg_relaxed(ptr, x) \
> +#define __xchg_relaxed(ptr, new, size) \
> + ___xchg(ptr, new, size, "", "", "")
> +
> +#define _arch_xchg(order, ptr, x) \
> ({ \
> __typeof__(*(ptr)) _x_ = (x); \
> - (__typeof__(*(ptr))) __xchg_relaxed((ptr), \
> - _x_, sizeof(*(ptr))); \
> + (__typeof__(*(ptr))) __xchg ## order((ptr), \
> + _x_, sizeof(*(ptr))); \
> })
>
> +#define arch_xchg_relaxed(ptr, x) \
> + _arch_xchg(_relaxed, ptr, x)
> +
> #define __xchg_acquire(ptr, new, size) \
> -({ \
> - __typeof__(ptr) __ptr = (ptr); \
> - __typeof__(new) __new = (new); \
> - __typeof__(*(ptr)) __ret; \
> - switch (size) { \
> - case 4: \
> - __asm__ __volatile__ ( \
> - " amoswap.w %0, %2, %1\n" \
> - RISCV_ACQUIRE_BARRIER \
> - : "=r" (__ret), "+A" (*__ptr) \
> - : "r" (__new) \
> - : "memory"); \
> - break; \
> - case 8: \
> - __asm__ __volatile__ ( \
> - " amoswap.d %0, %2, %1\n" \
> - RISCV_ACQUIRE_BARRIER \
> - : "=r" (__ret), "+A" (*__ptr) \
> - : "r" (__new) \
> - : "memory"); \
> - break; \
> - default: \
> - BUILD_BUG(); \
> - } \
> - __ret; \
> -})
> + ___xchg(ptr, new, size, "", "", RISCV_ACQUIRE_BARRIER)
>
> #define arch_xchg_acquire(ptr, x) \
> -({ \
> - __typeof__(*(ptr)) _x_ = (x); \
> - (__typeof__(*(ptr))) __xchg_acquire((ptr), \
> - _x_, sizeof(*(ptr))); \
> -})
> + _arch_xchg(_acquire, ptr, x)
>
> #define __xchg_release(ptr, new, size) \
> -({ \
> - __typeof__(ptr) __ptr = (ptr); \
> - __typeof__(new) __new = (new); \
> - __typeof__(*(ptr)) __ret; \
> - switch (size) { \
> - case 4: \
> - __asm__ __volatile__ ( \
> - RISCV_RELEASE_BARRIER \
> - " amoswap.w %0, %2, %1\n" \
> - : "=r" (__ret), "+A" (*__ptr) \
> - : "r" (__new) \
> - : "memory"); \
> - break; \
> - case 8: \
> - __asm__ __volatile__ ( \
> - RISCV_RELEASE_BARRIER \
> - " amoswap.d %0, %2, %1\n" \
> - : "=r" (__ret), "+A" (*__ptr) \
> - : "r" (__new) \
> - : "memory"); \
> - break; \
> - default: \
> - BUILD_BUG(); \
> - } \
> - __ret; \
> -})
> + ___xchg(ptr, new, size, "", RISCV_RELEASE_BARRIER, "")
>
> #define arch_xchg_release(ptr, x) \
> -({ \
> - __typeof__(*(ptr)) _x_ = (x); \
> - (__typeof__(*(ptr))) __xchg_release((ptr), \
> - _x_, sizeof(*(ptr))); \
> -})
> + _arch_xchg(_release, ptr, x)
>
> #define __xchg(ptr, new, size) \
> -({ \
> - __typeof__(ptr) __ptr = (ptr); \
> - __typeof__(new) __new = (new); \
> - __typeof__(*(ptr)) __ret; \
> - switch (size) { \
> - case 4: \
> - __asm__ __volatile__ ( \
> - " amoswap.w.aqrl %0, %2, %1\n" \
> - : "=r" (__ret), "+A" (*__ptr) \
> - : "r" (__new) \
> - : "memory"); \
> - break; \
> - case 8: \
> - __asm__ __volatile__ ( \
> - " amoswap.d.aqrl %0, %2, %1\n" \
> - : "=r" (__ret), "+A" (*__ptr) \
> - : "r" (__new) \
> - : "memory"); \
> - break; \
> - default: \
> - BUILD_BUG(); \
> - } \
> - __ret; \
> -})
> + ___xchg(ptr, new, size, ".aqrl", "", "")
>
> #define arch_xchg(ptr, x) \
> -({ \
> - __typeof__(*(ptr)) _x_ = (x); \
> - (__typeof__(*(ptr))) __xchg((ptr), _x_, sizeof(*(ptr))); \
> -})
> + _arch_xchg(, ptr, x)
>
> #define xchg32(ptr, x) \
> ({ \
> --
> 2.40.0
>
Reviewed-by: Guo Ren <guoren@xxxxxxxxxx>
--
Best Regards
Guo Ren