Re: [PATCH v3 1/3] libatomic: atomic_16.S: Improve ENTRY, END and ALIAS macro interface

[Victor Do Nascimento]

On 1/5/24 11:10, Richard Sandiford wrote:
Victor Do Nascimento <victor.donascime...@arm.com> writes:
The introduction of further architectural-feature dependent ifuncs
for AArch64 makes hard-coding ifunc `_i<n>' suffixes to functions
cumbersome to work with.  It is awkward to remember which ifunc maps
onto which arch feature and makes the code harder to maintain when new
ifuncs are added and their suffixes possibly altered.

This patch uses pre-processor `#define' statements to map each suffix to
a descriptive feature name macro, for example:

   #define LSE2 _i1

and reconstructs function names with the pre-processor's token
concatenation feature, such that for `MACRO(<name>_i<n>)', we would
now have `MACRO_FEAT(name, feature)' and in the macro definition body
we replace `name` with `name##feature`.

FWIW, another way of doing this would be to have:

#define CORE(NAME) NAME
#define LSE2(NAME) NAME##_i1

and use feature(name) instead of name##feature.  This has the slight
advantage of not using ## on empty tokens, and the maybe slightly
better advantage of not needing the extra forwarding step in:

#define ENTRY_FEAT(name, feat)          \
        ENTRY_FEAT1(name, feat)

#define ENTRY_FEAT1(name, feat) \

WDYT?

Richard


While from a strictly stylistic point of view, I'm not so keen on the 
resulting interface and its 'function call within a function call' look, 
e.g.

  ENTRY (LSE2 (libat_compare_exchange_16))

and

  ALIAS (LSE128 (libat_compare_exchange_16), \
         LSE2 (libat_compare_exchange_16))

on the implementation-side of things, I like the benefits this brings 
about.  Namely allowing the use of the unaltered original 
implementations of the ENTRY, END and ALIAS macros with the 
aforementioned advantages of not having to use ## on empty tokens and 
abolishing the need for the extra forwarding step.

I'm happy enough to go with this approach.

Cheers

Consequently, for base functionality, where the ifunc suffix is
absent, the macro interface remains the same.  For example, the entry
and endpoints of `libat_store_16' remain defined by:

   - ENTRY (libat_store_16)
and
   - END (libat_store_16)

For the LSE2 implementation of the same 16-byte atomic store, we now
have:

   - ENTRY_FEAT (libat_store_16, LSE2)
and
   - END_FEAT (libat_store_16, LSE2)

For the alising of ifunc names, we define the following new
implementation of the ALIAS macro:

   - ALIAS (FN_BASE_NAME, FROM_SUFFIX, TO_SUFFIX)

Defining the base feature name macro to map `CORE' to the empty string,
mapping LSE2 to the base implementation, we'd alias the LSE2
`libat_exchange_16' to it base implementation with:

   - ALIAS (libat_exchange_16, LSE2, CORE)

libatomic/ChangeLog:
        * config/linux/aarch64/atomic_16.S (CORE): New macro.
        (LSE2): Likewise.
        (ENTRY_FEAT): Likewise.
        (END_FEAT): Likewise.
        (ENTRY_FEAT1): Likewise.
        (END_FEAT1): Likewise.
        (ALIAS): Modify macro to take in `arch' arguments.
---
  libatomic/config/linux/aarch64/atomic_16.S | 83 +++++++++++++---------
  1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/libatomic/config/linux/aarch64/atomic_16.S 
b/libatomic/config/linux/aarch64/atomic_16.S
index a099037179b..eb8e749b8a2 100644
--- a/libatomic/config/linux/aarch64/atomic_16.S
+++ b/libatomic/config/linux/aarch64/atomic_16.S
@@ -40,22 +40,38 @@
  
  	.arch	armv8-a+lse
  
-#define ENTRY(name)		\
-       .global name;           \
-       .hidden name;           \
-       .type name,%function;   \
-       .p2align 4;             \
-name:                          \
-       .cfi_startproc;         \
+#define ENTRY(name) ENTRY_FEAT (name, CORE)
+
+#define ENTRY_FEAT(name, feat)         \
+       ENTRY_FEAT1(name, feat)
+
+#define ENTRY_FEAT1(name, feat)        \
+       .global name##feat;             \
+       .hidden name##feat;             \
+       .type name##feat,%function;     \
+       .p2align 4;                     \
+name##feat:                            \
+       .cfi_startproc;                 \
        hint    34      // bti c
  
-#define END(name)		\
-       .cfi_endproc;           \
-       .size name, .-name;
+#define END(name) END_FEAT (name, CORE)
  
-#define ALIAS(alias,name)	\
-       .global alias;          \
-       .set alias, name;
+#define END_FEAT(name, feat)                   \
+       END_FEAT1(name, feat)
+
+#define END_FEAT1(name, feat)          \
+       .cfi_endproc;                   \
+       .size name##feat, .-name##feat;
+
+#define ALIAS(alias, from, to)         \
+       ALIAS1(alias,from,to)
+
+#define ALIAS1(alias, from, to)                \
+       .global alias##from;            \
+       .set alias##from, alias##to;
+
+#define CORE
+#define LSE2   _i1
  
  #define res0 x0
  #define res1 x1
@@ -108,7 +124,7 @@ ENTRY (libat_load_16)
  END (libat_load_16)
  
  
-ENTRY (libat_load_16_i1)
+ENTRY_FEAT (libat_load_16, LSE2)
        cbnz    w1, 1f
  
  	/* RELAXED.  */
@@ -128,7 +144,7 @@ ENTRY (libat_load_16_i1)
        ldp     res0, res1, [x0]
        dmb     ishld
        ret
-END (libat_load_16_i1)
+END_FEAT (libat_load_16, LSE2)
  
  
  ENTRY (libat_store_16)
@@ -148,7 +164,7 @@ ENTRY (libat_store_16)
  END (libat_store_16)
  
  
-ENTRY (libat_store_16_i1)
+ENTRY_FEAT (libat_store_16, LSE2)
        cbnz    w4, 1f
  
  	/* RELAXED.  */
@@ -160,7 +176,7 @@ ENTRY (libat_store_16_i1)
        stlxp   w4, in0, in1, [x0]
        cbnz    w4, 1b
        ret
-END (libat_store_16_i1)
+END_FEAT (libat_store_16, LSE2)
  
  
  ENTRY (libat_exchange_16)
@@ -237,7 +253,7 @@ ENTRY (libat_compare_exchange_16)
  END (libat_compare_exchange_16)
  
  
-ENTRY (libat_compare_exchange_16_i1)
+ENTRY_FEAT (libat_compare_exchange_16, LSE2)
        ldp     exp0, exp1, [x1]
        mov     tmp0, exp0
        mov     tmp1, exp1
@@ -270,7 +286,7 @@ ENTRY (libat_compare_exchange_16_i1)
        /* ACQ_REL/SEQ_CST.  */
  4:    caspal  exp0, exp1, in0, in1, [x0]
        b       0b
-END (libat_compare_exchange_16_i1)
+END_FEAT (libat_compare_exchange_16, LSE2)
  
  
  ENTRY (libat_fetch_add_16)
@@ -556,21 +572,20 @@ END (libat_test_and_set_16)
  
  /* Alias entry points which are the same in baseline and LSE2.  */
  
-ALIAS (libat_exchange_16_i1, libat_exchange_16)
-ALIAS (libat_fetch_add_16_i1, libat_fetch_add_16)
-ALIAS (libat_add_fetch_16_i1, libat_add_fetch_16)
-ALIAS (libat_fetch_sub_16_i1, libat_fetch_sub_16)
-ALIAS (libat_sub_fetch_16_i1, libat_sub_fetch_16)
-ALIAS (libat_fetch_or_16_i1, libat_fetch_or_16)
-ALIAS (libat_or_fetch_16_i1, libat_or_fetch_16)
-ALIAS (libat_fetch_and_16_i1, libat_fetch_and_16)
-ALIAS (libat_and_fetch_16_i1, libat_and_fetch_16)
-ALIAS (libat_fetch_xor_16_i1, libat_fetch_xor_16)
-ALIAS (libat_xor_fetch_16_i1, libat_xor_fetch_16)
-ALIAS (libat_fetch_nand_16_i1, libat_fetch_nand_16)
-ALIAS (libat_nand_fetch_16_i1, libat_nand_fetch_16)
-ALIAS (libat_test_and_set_16_i1, libat_test_and_set_16)
-
+ALIAS (libat_exchange_16, LSE2, CORE)
+ALIAS (libat_fetch_add_16, LSE2, CORE)
+ALIAS (libat_add_fetch_16, LSE2, CORE)
+ALIAS (libat_fetch_sub_16, LSE2, CORE)
+ALIAS (libat_sub_fetch_16, LSE2, CORE)
+ALIAS (libat_fetch_or_16, LSE2, CORE)
+ALIAS (libat_or_fetch_16, LSE2, CORE)
+ALIAS (libat_fetch_and_16, LSE2, CORE)
+ALIAS (libat_and_fetch_16, LSE2, CORE)
+ALIAS (libat_fetch_xor_16, LSE2, CORE)
+ALIAS (libat_xor_fetch_16, LSE2, CORE)
+ALIAS (libat_fetch_nand_16, LSE2, CORE)
+ALIAS (libat_nand_fetch_16, LSE2, CORE)
+ALIAS (libat_test_and_set_16, LSE2, CORE)
  
  /* GNU_PROPERTY_AARCH64_* macros from elf.h for use in asm code.  */
  #define FEATURE_1_AND 0xc0000000