mbox series

[v2,0/4] Libatomic: Cleanup ifunc selector and aliasing

Message ID 20240611124250.1390575-1-victor.donascimento@arm.com
Headers show
Series Libatomic: Cleanup ifunc selector and aliasing | expand

Message

Victor Do Nascimento June 11, 2024, 12:42 p.m. UTC 
Changes in V2:

As explained in patch v2 1/4, it has become clear that the current
approach of querying assembler support for newer architectural
extensions at compile time is undesirable both from a maintainability
as well as a consistency standpoint - Different compiled versions of
Libatomic may have different features depending on the machine on
which they were built.

These issues make for difficult testing as the explosion in number of
`#ifdef' guards makes maintenance error-prone and the dependence on
binutils version means that, as well as deploying changes for testing
in a variety of target configurations, testing must also involve
compiling the library on an increasing number of host configurations,
meaning that the chance of bugs going undetected increases (as was
proved in the pre-commit CI which, due to the use of an older version
of Binutils, picked up on a runtime-error that had hitherto gone
unnoticed).

We therefore do away with the use of all assembly instructions
dependent on Binutils 2.42, choosing to replace them with `.inst's
instead.  This eliminates the latent bug picked up by CI and will
ensure consistent builds of Libatomic across all versions of Binutils.

---

The recent introduction of the optional LSE128 and RCPC3 architectural
extensions to AArch64 has further led to the increased flexibility of
atomic support in the architecture, with many extensions providing
support for distinct atomic operations, each with different potential
applications in mind.

This has led to maintenance difficulties in Libatomic, in particular
regarding the way the ifunc selector is generated via a series of
macro expansions at compile-time.

Until now, irrespective of the atomic operation in question, all atomic
functions for a particular operand size were expected to have the same
number of ifunc alternatives, meaning that a one-size-fits-all
approach could reasonably be taken for the selector.

This meant that if, hypothetically, for a particular architecture and
operand size one particular atomic operation was to have 3 different
implementations associated with different extensions, libatomic would
likewise be required to present three ifunc alternatives for all other
atomic functions.

The consequence in the design choice was the unnecessary use of
function aliasing and the unwieldy code which resulted from this.

This patch series attempts to remediate this issue by making the
preprocessor macros defining the number of ifunc alternatives and
their respective selection functions dependent on the file importing
the ifunc selector-generating framework.

all files are given `LAT_<FILENAME>' macros, defined at the beginning
and undef'd at the end of the file.  It is these macros that are
subsequently used to fine-tune the behaviors of `libatomic_i.h' and
`host-config.h'.

In particular, the definition of the `IFUNC_NCOND(N)' and
`IFUNC_COND_<n>' macros in host-config.h can now be guarded behind
these new file-specific macros, which ultimately control what the
`GEN_SELECTOR(X)' macro in `libatomic_i.h' expands to.  As both of
these headers are imported once per file implementing some atomic
operation, fine-tuned control is now possible.

Regtested with both `--enable-gnu-indirect-function' and
`--disable-gnu-indirect-function' configurations on armv9.4-a target
with LRCPC3 and LSE128 support and without.

Victor Do Nascimento (4):
  Libatomic: AArch64: Convert all lse128 assembly to .insn directives
  Libatomic: Define per-file identifier macros
  Libatomic: Make ifunc selector behavior contingent on importing file
  Libatomic: Clean up AArch64 `atomic_16.S' implementation file

 libatomic/acinclude.m4                       |  18 -
 libatomic/auto-config.h.in                   |   3 -
 libatomic/cas_n.c                            |   2 +
 libatomic/config/linux/aarch64/atomic_16.S   | 511 +++++++++----------
 libatomic/config/linux/aarch64/host-config.h |  35 +-
 libatomic/configure                          |  43 --
 libatomic/configure.ac                       |   3 -
 libatomic/exch_n.c                           |   2 +
 libatomic/fadd_n.c                           |   2 +
 libatomic/fand_n.c                           |   2 +
 libatomic/fence.c                            |   2 +
 libatomic/fenv.c                             |   2 +
 libatomic/fior_n.c                           |   2 +
 libatomic/flag.c                             |   2 +
 libatomic/fnand_n.c                          |   2 +
 libatomic/fop_n.c                            |   2 +
 libatomic/fsub_n.c                           |   2 +
 libatomic/fxor_n.c                           |   2 +
 libatomic/gcas.c                             |   2 +
 libatomic/gexch.c                            |   2 +
 libatomic/glfree.c                           |   2 +
 libatomic/gload.c                            |   2 +
 libatomic/gstore.c                           |   2 +
 libatomic/load_n.c                           |   2 +
 libatomic/store_n.c                          |   2 +
 libatomic/tas_n.c                            |   2 +
 26 files changed, 303 insertions(+), 350 deletions(-)

Comments

Richard Sandiford June 12, 2024, 8:50 a.m. UTC | #1 
Victor Do Nascimento <victor.donascimento@arm.com> writes:
> Changes in V2:
>
> As explained in patch v2 1/4, it has become clear that the current
> approach of querying assembler support for newer architectural
> extensions at compile time is undesirable both from a maintainability
> as well as a consistency standpoint - Different compiled versions of
> Libatomic may have different features depending on the machine on
> which they were built.
>
> These issues make for difficult testing as the explosion in number of
> `#ifdef' guards makes maintenance error-prone and the dependence on
> binutils version means that, as well as deploying changes for testing
> in a variety of target configurations, testing must also involve
> compiling the library on an increasing number of host configurations,
> meaning that the chance of bugs going undetected increases (as was
> proved in the pre-commit CI which, due to the use of an older version
> of Binutils, picked up on a runtime-error that had hitherto gone
> unnoticed).
>
> We therefore do away with the use of all assembly instructions
> dependent on Binutils 2.42, choosing to replace them with `.inst's
> instead.  This eliminates the latent bug picked up by CI and will
> ensure consistent builds of Libatomic across all versions of Binutils.

Nice!  Thanks for doing this.  It seems much cleaner and more flexible
than the current approach.

Thanks also for the clear organisation of the series.

OK for trunk.  (For the record, I didn't hand-check the encodings of the
.insts ...)

Richard

> ---
>
> The recent introduction of the optional LSE128 and RCPC3 architectural
> extensions to AArch64 has further led to the increased flexibility of
> atomic support in the architecture, with many extensions providing
> support for distinct atomic operations, each with different potential
> applications in mind.
>
> This has led to maintenance difficulties in Libatomic, in particular
> regarding the way the ifunc selector is generated via a series of
> macro expansions at compile-time.
>
> Until now, irrespective of the atomic operation in question, all atomic
> functions for a particular operand size were expected to have the same
> number of ifunc alternatives, meaning that a one-size-fits-all
> approach could reasonably be taken for the selector.
>
> This meant that if, hypothetically, for a particular architecture and
> operand size one particular atomic operation was to have 3 different
> implementations associated with different extensions, libatomic would
> likewise be required to present three ifunc alternatives for all other
> atomic functions.
>
> The consequence in the design choice was the unnecessary use of
> function aliasing and the unwieldy code which resulted from this.
>
> This patch series attempts to remediate this issue by making the
> preprocessor macros defining the number of ifunc alternatives and
> their respective selection functions dependent on the file importing
> the ifunc selector-generating framework.
>
> all files are given `LAT_<FILENAME>' macros, defined at the beginning
> and undef'd at the end of the file.  It is these macros that are
> subsequently used to fine-tune the behaviors of `libatomic_i.h' and
> `host-config.h'.
>
> In particular, the definition of the `IFUNC_NCOND(N)' and
> `IFUNC_COND_<n>' macros in host-config.h can now be guarded behind
> these new file-specific macros, which ultimately control what the
> `GEN_SELECTOR(X)' macro in `libatomic_i.h' expands to.  As both of
> these headers are imported once per file implementing some atomic
> operation, fine-tuned control is now possible.
>
> Regtested with both `--enable-gnu-indirect-function' and
> `--disable-gnu-indirect-function' configurations on armv9.4-a target
> with LRCPC3 and LSE128 support and without.
>
> Victor Do Nascimento (4):
>   Libatomic: AArch64: Convert all lse128 assembly to .insn directives
>   Libatomic: Define per-file identifier macros
>   Libatomic: Make ifunc selector behavior contingent on importing file
>   Libatomic: Clean up AArch64 `atomic_16.S' implementation file
>
>  libatomic/acinclude.m4                       |  18 -
>  libatomic/auto-config.h.in                   |   3 -
>  libatomic/cas_n.c                            |   2 +
>  libatomic/config/linux/aarch64/atomic_16.S   | 511 +++++++++----------
>  libatomic/config/linux/aarch64/host-config.h |  35 +-
>  libatomic/configure                          |  43 --
>  libatomic/configure.ac                       |   3 -
>  libatomic/exch_n.c                           |   2 +
>  libatomic/fadd_n.c                           |   2 +
>  libatomic/fand_n.c                           |   2 +
>  libatomic/fence.c                            |   2 +
>  libatomic/fenv.c                             |   2 +
>  libatomic/fior_n.c                           |   2 +
>  libatomic/flag.c                             |   2 +
>  libatomic/fnand_n.c                          |   2 +
>  libatomic/fop_n.c                            |   2 +
>  libatomic/fsub_n.c                           |   2 +
>  libatomic/fxor_n.c                           |   2 +
>  libatomic/gcas.c                             |   2 +
>  libatomic/gexch.c                            |   2 +
>  libatomic/glfree.c                           |   2 +
>  libatomic/gload.c                            |   2 +
>  libatomic/gstore.c                           |   2 +
>  libatomic/load_n.c                           |   2 +
>  libatomic/store_n.c                          |   2 +
>  libatomic/tas_n.c                            |   2 +
>  26 files changed, 303 insertions(+), 350 deletions(-)