Hello,
With Stage 3 nearing an end I was wondering what peoples perspective on
this patchset is.
Especially whether there's any chance of getting this in -- and what
should be done about the problematic codegen in target hooks when
compiling C++ (mentioned near the bottom of the original cover letter).
The SFINAE dependency is in, Prathamesh is working on
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81358 and that seems to be
nearing completion.
The libstdc++ dependency is upstream but not yet reviewed here:
https://gcc.gnu.org/pipermail/libstdc++/2025-January/060254.html (and I
would hope to get that in for GCC 15 no matter what happens with this
patch).
While this patchset has a few reviews from while in Stage 1 I'm not sure
what people's views on the likelyhood of getting this in are (mostly due
to lack of full review since the start of Stage 3).
On 11/14/24 13:55, mmalcom...@nvidia.com wrote:
From: Matthew Malcomson <mmalcom...@nvidia.com>
Hello,
This is the revision of the RFC posted here:
https://gcc.gnu.org/pipermail/gcc-patches/2024-September/663355.html
This patchset introduces floating point versions of atomic fetch_add,
fetch_sub, add_fetch and sub_fetch. Instructions for performing these
operations have been directly available in GPU hardware for a while, and
are now starting to get added to CPU ISA's with instructions like the
AArch64 LDFADD. Clang has allowed floating point types to be used with
these builtins for a while now https://reviews.llvm.org/D71726.
Introducing these new overloads to this builtin allows users to directly
specify the operation needed and hence allows the compiler to provide
optimised output if possible.
There is additional motivation to use such floating point type atomic
operations in libstdc++ so that other compilers can use libstdc++ to
generate optimal code for their own targets (e.g. NVC++ can use
libstdc++ atomic<float>::fetch_add to generate optimal code for GPU's
when using the `-stdpar` argument). A patch implementing that has been
sent to the libstdc++ mailing list here:
https://gcc.gnu.org/pipermail/libstdc++/2024-October/059865.html
Uses of such builtins in libstdc++ doesn't technically need introduction
of the builtins to GCC -- just ensuring that SFINAE techniques can be used
with overloaded builtins (as is done in this patch:
https://gcc.gnu.org/pipermail/gcc-patches/2024-October/665982.html).
That said it is still worthwhile to add these builtins -- especially now
that a primary target will be adding these operations.
N.b. I include an update to that libstdc++ patch in this patch series
because I noticed the ChangeLog in the cover letter had spaces instead
of tabs.
In the original RFC I suggested that libstdc++ use preprocessor checks
along the lines of `__has_builtin(__atomic_fetch_add_fp)` to determine
whether to use the new builtins or a CAS loop written inline. After asking
the clang community I found out that such resolved versions of the atomic
builtins are not exposed by clang to the user. They suggested the use of
SFINAE to determine if a given type works with the `__atomic_fetch_add`
builtin. That's why I have posted the SFINAE patch above and chosen this
approach.
------------------------------
As standard with the existing atomic builtins, we add the same functions
in libatomic, allowing a fallback for when a given target has not
implemented these operations directly in hardware. In order to use
these functions we need to have a naming scheme that encodes the type --
we use a suffix of _fp to denote that this operation is on a floating
point type, and a further empty suffix to denote a double, 'f' to denote
a float, and similar. The scheme for the second part of the suffix
taken from the existing builtins that have different versions for
different floating point types -- e.g. __builtin_acosh,
__builtin_acoshf, __builtin_acoshl, etc.
In order to add floating point functions to libatomic we updated the
makefile machinery to use names more descriptive of the new setup (where
the SIZE of the datatype can no longer be used to distinguish all
operations from each other). Moreover we add a CAS loop implementation
in fop_n.c that handles floating point exception information and handles
casting between floating point and integral types when switching between
applying the operation and using CAS to attempt to store.
------------------------------
As Joseph Myers pointed out in response to my RFC, when performing
floating point operations in a CAS loop there is the floating point
exception information to take care of. In order to take care of this
information I use the existing `atomic_assign_expand_fenv` target hook
to generate code that checks this information.
Partly due to the fact that this hook emits GENERIC code and partly due
to the language-specific semantics of floating point exceptions, this
means we now decide whether to emit a CAS loop handling the frontend
(during overload resolution). The frontend decides to only use the
underlying builtin if the backend has an optab defined that can
implement it directly.
------------------------------
Now that the expansion to a CAS loop is performed in overloaded builtin
resolution, this means that if the user were to directly use a resolved
version (e.g. `__atomic_fetch_add_fp` for a double) that would not
expand into a CAS loop inline. Instead (assuming the optab is not
implemented for this target) it would pass through and end up using the
libatomic fallback.
This is not ideal, but I believe the complexity of adding another clause
for this expansion to a CAS loop is not worth the benefit of handling a
CAS loop expansion for this specific case (partly on the assumption that
users would rarely specify the resolved version and partly on the belief
that these resolved versions are not actually part of the user-facing
interface -- since they're not documented in the manual and don't seem
to be used enough for clang to expose the interface).
I considered not exposing the resolved versions to the user (similar to
the interface that _BitInt exposes) and instead handling them as an
internal function that could expand to call the libatomic
implementation. I chose not to do that for consistency with the rest of
the atomic builtins.
------------------------------
There are a few places throughout the compiler that handle such atomic
builtins and I have not updated to handle floating point atomic
builtins. Places like asan, tsan, gimple-ssa-warn-access, analyzer, and
tree-ssa-forwprop would need to be updated eventually.
However since the current state of GCC is that no backend implements
these optabs directly the generic version of the builtin is always
expanded as a CAS loop in the frontend -- this means these mid-end
passes will not see any of these builtins except in the case that the
user explicitly calls the resolved version.
I hoping to update these places in a later patch.
------------------------------
N.b. there is a remaining bug where ix86_atomic_assign_expand_fenv
generates code that gets broken during optimisation by `fold`. I
believe the code returned by this function is incorrect (maybe only for
C++?). Hoping someone can confirm that.
The expression that gets incorrectly optimised is along the lines of:
COMPOUND_EXPR<TARGET_EXPR<var1, some-init>,
TARGET_EXPR<var2, expression-with-var1>>
and `fold` (which gets called by `cp_fold`) removes the first
TARGET_EXPR since it doesn't look like it has side effects (even though
the variable it sets is used in the second expression). I trialled
setting TREE_SIDE_EFFECTS in the hook and that appeared to work but
would appreciate feedback on whether that's the "correct" fix or not
(and if so confirmation that all relevant hooks should do something
similar).
Due to this currently C++ code using this builtin fails on x86_64.
libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc
------------------------------
N.b. the story around C++ is not quite finished yet. Automatic linking
in of libatomic is being worked on and we hope to send something
upstream this week. Similarly the target hooks that were written for C
seem to be problematic for C++ (as described in the point above).
The goal is still to include this feature (after having fixed the
problem outlined above) and having automatic linking of libatomic.
However, I have a few fallback approaches that I'd like to gather
opinions on (with the goal of those fallbacks to upstream the libstdc++
changes that I linked earlier in this message):
1) Disable fenv exception handling in the builtin for C++ code for this
release. (libstdc++ does not currently hande fenv exception, so
this would not be a regression in that code. I don't think adding
exception handling in later would be backwards incompatible).
2) Include the builtin-SFINAE patch and the libstdc++ patch (both
already upstreamed) into this release knowing that GCC itself will
not use this codepath.
------------------------------
Testing done:
Bootstrap and regression test passes (with known exceptions of C++ on
x86_64, and libatomic linking in general) on x86_64 and AArch64.
Cross compiler regression tests pass on arm-linux.
Also tested with the compiler installed to check behaviour when
libatomic is in the link path (to check that known failures from
libatomic linking go away when it's available).
Similarly tested with a dummy implementation of fetch_add as an optab
in the AArch64 backend to ensure that codepath also works.
------------------------------
Matthew Malcomson (11):
libatomic: Split concept of SUFFIX and SIZE in libatomic
libatomic: Add floating point implementations of fetch_{add,sub}
c: c++: Define new floating point builtin fetch_add functions
builtins: Add FP types for atomic builtin overload resolution
c: c++: Expand into CAS loop in frontend
builtins: optab: Tie the new atomic builtins to the backend
testsuite: Add tests for fp resolutions of __atomic_fetch_add
c: c++: flag to disable fetch_op handling fenv exceptions
doc: Mention floating point atomic fetch_add etc in docs
[Not For Commit] Add demo implementation of one of the operations
[Not For Commit] Link libatomic into tests when available
gcc/builtin-types.def | 20 +
gcc/builtins.cc | 176 ++
gcc/builtins.h | 2 +
gcc/c-family/c-common.cc | 205 ++-
gcc/c-family/c.opt | 6 +
gcc/config/aarch64/aarch64.h | 2 +
gcc/config/aarch64/aarch64.opt | 5 +
gcc/config/aarch64/atomics.md | 15 +
gcc/doc/extend.texi | 10 +
gcc/fortran/f95-lang.cc | 5 +
gcc/fortran/types.def | 17 +
gcc/optabs.cc | 19 +
gcc/optabs.def | 6 +-
gcc/sync-builtins.def | 40 +
.../template/builtin-atomic-overloads.def | 28 +-
.../template/builtin-atomic-overloads6.C | 23 +-
.../template/builtin-atomic-overloads7.C | 16 +-
gcc/testsuite/gcc.dg/atomic-op-fp-convert.c | 6 +
gcc/testsuite/gcc.dg/atomic-op-fp-errs.c | 14 +
gcc/testsuite/gcc.dg/atomic-op-fp-fenv-flag.c | 44 +
.../gcc.dg/atomic-op-fp-resolve-complain.c | 5 +
gcc/testsuite/gcc.dg/atomic-op-fp.c | 200 +++
gcc/testsuite/gcc.dg/atomic-op-fpf.c | 200 +++
gcc/testsuite/gcc.dg/atomic-op-fpf128.c | 203 +++
gcc/testsuite/gcc.dg/atomic-op-fpf16.c | 203 +++
gcc/testsuite/gcc.dg/atomic-op-fpf16b.c | 203 +++
gcc/testsuite/gcc.dg/atomic-op-fpf32.c | 203 +++
gcc/testsuite/gcc.dg/atomic-op-fpf32x.c | 203 +++
gcc/testsuite/gcc.dg/atomic-op-fpf64.c | 203 +++
gcc/testsuite/gcc.dg/atomic-op-fpf64x.c | 203 +++
gcc/testsuite/gcc.dg/atomic-op-fpl.c | 200 +++
.../gcc.dg/atomic/atomic-op-fp-fenv.c | 376 +++++
.../gcc.target/i386/excess-precision-13.c | 88 +
gcc/testsuite/lib/target-supports.exp | 199 ++-
libatomic/Makefile.am | 38 +-
libatomic/Makefile.in | 44 +-
libatomic/acinclude.m4 | 56 +-
libatomic/auto-config.h.in | 114 +-
libatomic/cas_n.c | 8 +-
libatomic/config/linux/aarch64/host-config.h | 23 +-
libatomic/config/linux/arm/host-config.h | 2 +-
libatomic/config/s390/cas_n.c | 6 +-
libatomic/config/s390/exch_n.c | 4 +-
libatomic/config/s390/load_n.c | 4 +-
libatomic/config/s390/store_n.c | 4 +-
libatomic/config/x86/host-config.h | 14 +-
libatomic/configure | 1485 ++++++++++++++++-
libatomic/configure.ac | 6 +
libatomic/exch_n.c | 12 +-
libatomic/fadd_n.c | 23 +-
libatomic/fop_n.c | 111 +-
libatomic/fsub_n.c | 23 +
libatomic/libatomic.map | 44 +
libatomic/libatomic_i.h | 186 ++-
libatomic/load_n.c | 12 +-
libatomic/store_n.c | 12 +-
libatomic/tas_n.c | 12 +-
libatomic/testsuite/Makefile.in | 1 +
.../testsuite/libatomic.c/atomic-op-fp-fenv.c | 421 +++++
.../testsuite/libatomic.c/atomic-op-fp.c | 219 +++
.../testsuite/libatomic.c/atomic-op-fpf.c | 219 +++
.../testsuite/libatomic.c/atomic-op-fpf128.c | 220 +++
.../testsuite/libatomic.c/atomic-op-fpf16.c | 223 +++
.../testsuite/libatomic.c/atomic-op-fpf16b.c | 220 +++
.../testsuite/libatomic.c/atomic-op-fpf32.c | 220 +++
.../testsuite/libatomic.c/atomic-op-fpf32x.c | 220 +++
.../testsuite/libatomic.c/atomic-op-fpf64.c | 220 +++
.../testsuite/libatomic.c/atomic-op-fpf64x.c | 220 +++
.../testsuite/libatomic.c/atomic-op-fpl.c | 219 +++
.../testsuite/29_atomics/atomic_float/1.cc | 1 +
.../testsuite/29_atomics/atomic_ref/float.cc | 1 +
71 files changed, 7978 insertions(+), 237 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fp-convert.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fp-errs.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fp-fenv-flag.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fp-resolve-complain.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fp.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fpf.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fpf128.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fpf16.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fpf16b.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fpf32.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fpf32x.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fpf64.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fpf64x.c
create mode 100644 gcc/testsuite/gcc.dg/atomic-op-fpl.c
create mode 100644 gcc/testsuite/gcc.dg/atomic/atomic-op-fp-fenv.c
create mode 100644 gcc/testsuite/gcc.target/i386/excess-precision-13.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fp-fenv.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fp.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fpf.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fpf128.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fpf16.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fpf16b.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fpf32.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fpf32x.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fpf64.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fpf64x.c
create mode 100644 libatomic/testsuite/libatomic.c/atomic-op-fpl.c
