Victor Do Nascimento <victor.donascime...@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(-)
