Thanks Jonathan, I agree with your point that having just the check against one of the overloaded versions is not very robust and having multiple checks against different versions would be better.
Unfortunately - while asking the clang folk about this I realised that clang doesn't expose the resolved versions (e.g. the existing versions like `__atomic_load_2` etc) to the user. Instead they allow using SFINAE on these overloaded builtins. https://discourse.llvm.org/t/atomic-floating-point-operations-and-libstdc/81461 I spent some time looking at this and it seems that enabling SFINAE in GCC for these builtins is not too problematic (idea being to pass in a `error_on_noresolve` boolean to `resolve_overloaded_builtin` based on the context in the C++ frontend, then only emit errors if that boolean is set). To Jonathon: * Would you be OK with using SFINAE to choose whether to use the __atomic_fetch_add builtin for floating point types in libstdc++? At C++ frontend maintainers I Cc'd in: * Are you happy with the idea of enabling SFINAE on overloaded builtins resolved via resolve_overloaded_builtin? To global maintainers I Cc'd in: * Is there any reason you know of not to enable SFINAE on the overloaded builtins? * Would it be OK to enable SFINAE on the generic overloaded builtins and add the parameter so that targets can do the same for their target-specific builtins (i.e. without changing the behaviour of the existing target specific builtins)? ________________________________ From: Jonathan Wakely <jwak...@redhat.com> Sent: 19 September 2024 3:47 PM To: Matthew Malcomson <mmalcom...@nvidia.com> Cc: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>; Joseph Myers <josmy...@redhat.com>; Richard Biener <rguent...@suse.de> Subject: Re: [PATCH 0/8] [RFC] Introduce floating point fetch_add builtins External email: Use caution opening links or attachments On Thu, 19 Sept 2024 at 14:12, <mmalcom...@nvidia.com> wrote: > > From: Matthew Malcomson <mmalcom...@nvidia.com> > > Hello, this is an RFC for adding an atomic floating point fetch_add builtin > (and variants) to GCC. The atomic fetch_add operation is defined to work > on the base floating point types in the C++20 standard chapter 31.7.3, and > extended to work for all cv-unqualified floating point types in C++23 > chapter 33.5.7.4. > > Honestly not sure who to Cc, please do point me to someone else if that's > better. > > This is nowhere near complete (for one thing even the tests I've added > don't fully pass), but I think I have a complete enough idea that it's > worth checking if this is something that could be agreed on. > > As it stands no target except the nvptx backend would natively support > these operations. > > Main questions that I'm looking to resolve with this RFC: > 1) Would GCC be OK accepting this implementation even though no backend > would be implementing these yet? > - AIUI only the nvptx backend could theoretically implement this. > - Even without a backend implementing it natively, the ability to use > this in code (especially libstdc++) enables other compilers to > generate better code for GPU's using standard C++. > 2) Would libstdc++ be OK relying on `__has_builtin(__atomic_fetch_add_fp)` > (i.e. a check on the resolved builtin rather than the more user-facing > one) in order to determine whether floating point atomic fetch_add is > available. Yes, if that name is what other compilers will also use (have you discussed this with Clang?) It looks like PATCH 5/8 only uses the _fp name for fetch_add though, and just uses fetch_sub etc. for the other functions, is that a mistake? > - N.b. this builtin is actually the builtin working on the "double" OK, so the library code just calls the generic __atomic_fetch_add that accepts any types, but then that gets expanded to a more specific form for float, double etc.? And the more specific form has to exist at some level, because we need an extern symbol from libatomic, so either we include the type as an explicit suffix on the name, or we use some kind of name mangling like _Z18__atomic_fetch_addPdS_S_, which is obviously nasty. > type, one would have to rely on any compilers implementing that > particular resolved builtin to also implement the other floating point > atomic fetch_add builtins that they would want to support in libstdc++ > `atomic<[floating_point_type]>::fetch_add`. This seems a bit concerning. I can imagine somebody implementing these for float and double first, but leaving long double, _Float64, _Float32, _Float128 etc. for later. In that case, libstdc++ would not work if somebody tries to use std::atomic<long double>, or whichever types aren't supported yet. It's OK if we can be *sure* that won't happen i.e. that Clang will either implement the new built-in for *all* FP types, or none. > > More specific questions about the choice of which builtins to implement and > whether the types are OK: > 1) Is it OK to not implement the `__sync_*` versions? > Since these are deprecated and the `__atomic_*` versions are there to > match the C/C++ code atomic operations (which is a large part of the > reason for the new floating point operations). > 2) Is it OK to not implement all the `fetch_*` operations? > None of the bitwise operations are specified for C++ and bitwise > operations are (AIUI) rarely used on floating point values. That seems OK (entirely correct even) to me. > 3) Wanting to be able to farm out to libatomic meant that we need constant > names > for the specialised functions. > - This led to the naming convention based on floating point type. > - That naming convention *could* be updated to include the special backend > floating point types if needed. I have not done this mostly because I > thought it would not add much, though I have not looked into this very > closely. > 4) Wanting to name the functions based on floating point type rather than size > meant that the mapping from type passed to the overloaded version to > specialised builtin was less direct than for the integral versions. > - Ended up with a hard-coded table in the source to check this. > - Would very much like some better approach, not certain what better > approach > I could find. > - Will eventually at least share the hard-coded tables (which isn't > happening yet because this is at RFC level). > 5) Are there any other types that I should use? > Similarly are there any types that I'm trying to use that I shouldn't? > I *believe* I've implemented all the types that make sense and are > general builtin types. Could easily have missed some (e.g. left > `_Float128x` alone because AIUI it's larger than 128bits which means we > don't have any other atomic operations on such data), could also easily That seems like a problem though - it means that GCC could be in exactly the concerning position described above: there could be a FP type (_Float128x) for which we have no __atomic_fetch_add built-in, but the __has_builtin(__atomic_fetch_add) check would work, because the built-in for double exists. So std::atomic<_Float128x> would not work. I don't think that's a problem today, because we don't have a _Float128x type AFAIK. But that suggests to me that the rule for whether to define the built-in for a FP type needs to be based on whether the type exists at all, not whether we have other atomic ops for it. Maybe the libstdc++ code should have a separate __has_builtin check for each type, e.g. bool __use_builtin = false; +#if __has_builtin(__atomic_fetch_add_fp) + if constexpr (is_same_v<_Tp, double>) + __use_builtin = true; +##endif +#if __has_builtin(__atomic_fetch_add_fpf) + if constexpr (is_same_v<_Tp, float>) + __use_builtin = true; +##endif +#if __has_builtin(__atomic_fetch_add_fpl) + if constexpr (is_same_v<_Tp, long double>) + __use_builtin = true; +##endif // ... if (__use_builtin) return __atomic_fatch_add(...); // .. current CAS-based implementation And so on with __has_builtin checks for the function for each type. This would be a lot more tedious, but would handle the case where the new built-in is only supported for some types. > be misunderstanding the mention in the C++ standards of "extended" types > (which I believe is the `_Float*` and `bfloat16` types). Yes. The C++ standard says there's a typedef std::float32_t which denotes some implementation-defined type, which is _Float32 in our implementation. But the _Float32 name is not in the C++ standard. > 6) Anything special about floating point maths that I'm tripping up on? > (Especially around CAS loop where we expand the operation directly, > sometimes convert a PLUS into a MINUS of a negative value ...). > Don't know of anything myself, but also a bit wary of floating point > maths. > > N.b. I know that there's a reasonable amount of work left in: > 1) Ensuring that all the places which use atomic types are updated > (e.g. asan), > 2) Ensuring that all frontends can use these to the level that they could > use the integral atomics. > 3) Ensuring the major backends can still compile libatomic (I had to do > something in AArch64 libatomic backend to make it compile, seems like > there might be more to do for others). > > > Matthew Malcomson (8): > Define new floating point builtin fetch_add functions > Add FP types for atomic builtin overload resolution > Tie the new atomic builtins to the backend > Have libatomic working as first draft > Use new builtins in libstdc++ > First attempt at testsuite > Mention floating point atomic fetch_add etc in docs > Add demo implementation of one of the operations > > gcc/builtin-types.def | 20 + > gcc/builtins.cc | 153 ++- > gcc/c-family/c-common.cc | 88 +- > gcc/config/aarch64/aarch64.h | 2 + > gcc/config/aarch64/aarch64.opt | 5 + > gcc/config/aarch64/atomics.md | 15 + > gcc/doc/extend.texi | 12 + > gcc/fortran/types.def | 48 + > gcc/optabs.cc | 101 +- > gcc/optabs.def | 6 +- > gcc/optabs.h | 2 +- > gcc/sync-builtins.def | 40 + > gcc/testsuite/gcc.dg/atomic-op-fp.c | 204 +++ > gcc/testsuite/gcc.dg/atomic-op-fpf.c | 204 +++ > gcc/testsuite/gcc.dg/atomic-op-fpf128.c | 204 +++ > gcc/testsuite/gcc.dg/atomic-op-fpf16.c | 204 +++ > gcc/testsuite/gcc.dg/atomic-op-fpf16b.c | 204 +++ > gcc/testsuite/gcc.dg/atomic-op-fpf32.c | 204 +++ > gcc/testsuite/gcc.dg/atomic-op-fpf32x.c | 204 +++ > gcc/testsuite/gcc.dg/atomic-op-fpf64.c | 204 +++ > gcc/testsuite/gcc.dg/atomic-op-fpf64x.c | 204 +++ > gcc/testsuite/gcc.dg/atomic-op-fpl.c | 204 +++ > gcc/testsuite/lib/target-supports.exp | 463 ++++++- > libatomic/Makefile.am | 6 +- > libatomic/Makefile.in | 12 +- > libatomic/acinclude.m4 | 49 + > libatomic/auto-config.h.in | 84 +- > libatomic/config/linux/aarch64/host-config.h | 2 + > libatomic/configure | 1153 ++++++++++++++++- > libatomic/configure.ac | 4 + > libatomic/fadd_n.c | 23 + > libatomic/fop_n.c | 5 +- > libatomic/fsub_n.c | 23 + > libatomic/libatomic.map | 44 + > libatomic/libatomic_i.h | 58 + > libatomic/testsuite/Makefile.in | 1 + > .../testsuite/libatomic.c/atomic-op-fp.c | 203 +++ > .../testsuite/libatomic.c/atomic-op-fpf.c | 203 +++ > .../testsuite/libatomic.c/atomic-op-fpf128.c | 203 +++ > .../testsuite/libatomic.c/atomic-op-fpf16.c | 203 +++ > .../testsuite/libatomic.c/atomic-op-fpf16b.c | 203 +++ > .../testsuite/libatomic.c/atomic-op-fpf32.c | 203 +++ > .../testsuite/libatomic.c/atomic-op-fpf32x.c | 203 +++ > .../testsuite/libatomic.c/atomic-op-fpf64.c | 203 +++ > .../testsuite/libatomic.c/atomic-op-fpf64x.c | 203 +++ > .../testsuite/libatomic.c/atomic-op-fpl.c | 203 +++ > libstdc++-v3/include/bits/atomic_base.h | 16 + > 47 files changed, 6393 insertions(+), 112 deletions(-) > 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 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 > > -- > 2.43.0 >
