[0/8,RFC] Introduce floating point fetch_add builtins

Message ID 20240919131204.3865854-1-mmalcomson@nvidia.com
Headers
Series Introduce floating point fetch_add builtins |

Message

Matthew Malcomson Sept. 19, 2024, 1:11 p.m. UTC
  From: Matthew Malcomson <mmalcomson@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.
   - N.b. this builtin is actually the builtin working on the "double"
     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`.

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.
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
   be misunderstanding the mention in the C++ standards of "extended" types
   (which I believe is the `_Float*` and `bfloat16` types).
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
  

Comments

Jonathan Wakely Sept. 19, 2024, 2:47 p.m. UTC | #1
On Thu, 19 Sept 2024 at 14:12, <mmalcomson@nvidia.com> wrote:
>
> From: Matthew Malcomson <mmalcomson@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
>
  
Joseph Myers Sept. 19, 2024, 9:38 p.m. UTC | #2
On Thu, 19 Sep 2024, mmalcomson@nvidia.com wrote:

> 6) Anything special about floating point maths that I'm tripping up on?

Correct atomic operations with floating-point operands should ensure that 
exceptions raised exactly correspond to the operands for which the 
operation succeeded, and not to the operands for any previous attempts 
where the compare-exchange failed.  There is a lengthy note in the C 
standard (in C11 it's a footnote in 6.5.16.2, in C17 it's a Note in 
6.5.16.2 and in C23 that subclause has become 6.5.17.3) that discusses 
appropriate code sequences to achieve this.  In GCC the implementation of 
this is in c-typeck.cc:build_atomic_assign, which in turn calls 
targetm.atomic_assign_expand_fenv (note that we have the complication for 
C of not introducing libm dependencies in code that only uses standard 
language features and not <math.h>, <fenv.h> or <complex.h>, so direct use 
of <fenv.h> functions is inappropriate here).

I would expect such built-in functions to follow the same semantics for 
floating-point exceptions as _Atomic compound assignment does.  (Note that 
_Atomic compound assignment is more general in the allowed operands, 
because compound assignment is a heterogeneous operation - for example, 
the special floating-point logic in build_atomic_assign includes the case 
where the LHS of the compound assignment is of atomic integer type but the 
RHS is of floating type.  However, built-in functions allow memory orders 
other than seq_cst to be used, whereas _Atomic compound assignment is 
limited to the seq_cst case.)

So it would seem appropriate for the implementation of such built-in 
functions to make use of targetm.atomic_assign_expand_fenv for 
floating-point environment handling, and for testcases to include tests 
analogous to c11-atomic-exec-5.c that exceptions are being handled 
correctly.

Cf. N2329 which suggested such operations for C in <stdatomic.h> (but 
tried to do to many things in one paper to be accepted into C); it didn't 
go into the floating-point exceptions semantics but simple correctness 
would indicate avoiding spurious exceptions from discarded computations.
  
Matthew Malcomson Oct. 2, 2024, 4:48 p.m. UTC | #3
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)?
  
Jonathan Wakely Oct. 2, 2024, 7:40 p.m. UTC | #4
On Wed, 2 Oct 2024 at 17:48, Matthew Malcomson <mmalcomson@nvidia.com> wrote:
>
> 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++?

It's not great for compile times, but no objection otherwise.

>
>
> 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 <jwakely@redhat.com>
> Sent: 19 September 2024 3:47 PM
> To: Matthew Malcomson <mmalcomson@nvidia.com>
> Cc: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>; Joseph Myers <josmyers@redhat.com>; Richard Biener <rguenther@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, <mmalcomson@nvidia.com> wrote:
> >
> > From: Matthew Malcomson <mmalcomson@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
> >
>
  
Matthew Malcomson Oct. 14, 2024, 1 p.m. UTC | #5
After getting GCC to handle floating point exceptions I'm now noticing that the `targetm.atomic_assign_expand_fenv` hook tends to use the library function `__atomic_feraiseexcept`.
This requires adding -latomic to the command line where it wasn't needed before.
I also notice that the inline CAS loop for libstdc++ `atomic<T>::fetch_add` on floating point types doesn't handle these floating point exceptions.

For context, clang (which has already implemented floating point fetch_add for floating point types) is not emitting anything to handle the floating point environment.

I expect that the eventual goal should be (please do correct if you disagree)

  1.
Update the existing libstdc++ CAS loop to handle floating point exceptions.
  2.
Suggest to clang that the fetch_add builtin should handle floating point exceptions.
  3.
GCC builtin handles floating point exceptions.
  4.
__atomic_feraiseexcept should be a builtin to avoid previously unnecessary requirement to link libatomic.

However I'm uncertain we could sort all that out for GCC 15.
Would it be reasonable to require linking with libatomic for the GCC 15 release?
Alternatively is there some other intermediate step that people would prefer?
Similarly — maybe there's something I'm missing — maybe about differences between C/C++?

Another option is to aim for implementing the functionality of __atomic_feraiseexcept inline in the fetch_add GCC 15 (a bit tight on time), to use it in the new fetch_add builtin overload, but not use it in the CAS loop.
That way no compiler & floating point type combination would need linking libatomic, current clang would not handle floating point exception information, while current gcc would handle that information.

What are your thoughts?
Regards,
Matthew
  
Jonathan Wakely Oct. 14, 2024, 1:35 p.m. UTC | #6
N.B. please CC the libstdc++ list for libstdc++ topics.

On Mon, 14 Oct 2024 at 14:00, Matthew Malcomson <mmalcomson@nvidia.com> wrote:
>
> After getting GCC to handle floating point exceptions I'm now noticing that the `targetm.atomic_assign_expand_fenv` hook tends to use the library function `__atomic_feraiseexcept`.
> This requires adding -latomic to the command line where it wasn't needed before.
> I also notice that the inline CAS loop for libstdc++ `atomic<T>::fetch_add` on floating point types doesn't handle these floating point exceptions.

Right, Joseph made the C front-end handle them for _Atomic but I
didn't do that for std::atomic.

> For context, clang (which has already implemented floating point fetch_add for floating point types) is not emitting anything to handle the floating point environment.
>
> I expect that the eventual goal should be (please do correct if you disagree)
>
> Update the existing libstdc++ CAS loop to handle floating point exceptions.
> Suggest to clang that the fetch_add builtin should handle floating point exceptions.
> GCC builtin handles floating point exceptions.
> __atomic_feraiseexcept should be a builtin to avoid previously unnecessary requirement to link libatomic.
>
> However I'm uncertain we could sort all that out for GCC 15.
> Would it be reasonable to require linking with libatomic for the GCC 15 release?

That would be a regression for anybody using
std::atomic<floating-point-type> today.

> Alternatively is there some other intermediate step that people would prefer?
> Similarly — maybe there's something I'm missing — maybe about differences between C/C++?
>
> Another option is to aim for implementing the functionality of __atomic_feraiseexcept inline in the fetch_add GCC 15 (a bit tight on time), to use it in the new fetch_add builtin overload, but not use it in the CAS loop.
> That way no compiler & floating point type combination would need linking libatomic, current clang would not handle floating point exception information, while current gcc would handle that information.
>
> What are your thoughts?

If making it Just Work without a new libatomic requirement is
possible, I'd prefer that.

> Regards,
> Matthew
>
> ________________________________
> From: Joseph Myers <josmyers@redhat.com>
> Sent: 19 September 2024 10:38 PM
> To: Matthew Malcomson <mmalcomson@nvidia.com>
> Cc: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>; Jonathan Wakely <jwakely@redhat.com>; Richard Biener <rguenther@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 Sep 2024, mmalcomson@nvidia.com wrote:
>
> > 6) Anything special about floating point maths that I'm tripping up on?
>
> Correct atomic operations with floating-point operands should ensure that
> exceptions raised exactly correspond to the operands for which the
> operation succeeded, and not to the operands for any previous attempts
> where the compare-exchange failed.  There is a lengthy note in the C
> standard (in C11 it's a footnote in 6.5.16.2, in C17 it's a Note in
> 6.5.16.2 and in C23 that subclause has become 6.5.17.3) that discusses
> appropriate code sequences to achieve this.  In GCC the implementation of
> this is in c-typeck.cc:build_atomic_assign, which in turn calls
> targetm.atomic_assign_expand_fenv (note that we have the complication for
> C of not introducing libm dependencies in code that only uses standard
> language features and not <math.h>, <fenv.h> or <complex.h>, so direct use
> of <fenv.h> functions is inappropriate here).
>
> I would expect such built-in functions to follow the same semantics for
> floating-point exceptions as _Atomic compound assignment does.  (Note that
> _Atomic compound assignment is more general in the allowed operands,
> because compound assignment is a heterogeneous operation - for example,
> the special floating-point logic in build_atomic_assign includes the case
> where the LHS of the compound assignment is of atomic integer type but the
> RHS is of floating type.  However, built-in functions allow memory orders
> other than seq_cst to be used, whereas _Atomic compound assignment is
> limited to the seq_cst case.)
>
> So it would seem appropriate for the implementation of such built-in
> functions to make use of targetm.atomic_assign_expand_fenv for
> floating-point environment handling, and for testcases to include tests
> analogous to c11-atomic-exec-5.c that exceptions are being handled
> correctly.
>
> Cf. N2329 which suggested such operations for C in <stdatomic.h> (but
> tried to do to many things in one paper to be accepted into C); it didn't
> go into the floating-point exceptions semantics but simple correctness
> would indicate avoiding spurious exceptions from discarded computations.
>
> --
> Joseph S. Myers
> josmyers@redhat.com
>
  
Jonathan Wakely Oct. 14, 2024, 1:36 p.m. UTC | #7
On Mon, 14 Oct 2024 at 14:35, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> N.B. please CC the libstdc++ list for libstdc++ topics.
>
> On Mon, 14 Oct 2024 at 14:00, Matthew Malcomson <mmalcomson@nvidia.com> wrote:
> >
> > After getting GCC to handle floating point exceptions I'm now noticing that the `targetm.atomic_assign_expand_fenv` hook tends to use the library function `__atomic_feraiseexcept`.
> > This requires adding -latomic to the command line where it wasn't needed before.
> > I also notice that the inline CAS loop for libstdc++ `atomic<T>::fetch_add` on floating point types doesn't handle these floating point exceptions.
>
> Right, Joseph made the C front-end handle them for _Atomic but I
> didn't do that for std::atomic.
>
> > For context, clang (which has already implemented floating point fetch_add for floating point types) is not emitting anything to handle the floating point environment.

I'm not sure if C++ users really care about fenv. I think most don't.


> >
> > I expect that the eventual goal should be (please do correct if you disagree)
> >
> > Update the existing libstdc++ CAS loop to handle floating point exceptions.
> > Suggest to clang that the fetch_add builtin should handle floating point exceptions.
> > GCC builtin handles floating point exceptions.
> > __atomic_feraiseexcept should be a builtin to avoid previously unnecessary requirement to link libatomic.
> >
> > However I'm uncertain we could sort all that out for GCC 15.
> > Would it be reasonable to require linking with libatomic for the GCC 15 release?
>
> That would be a regression for anybody using
> std::atomic<floating-point-type> today.
>
> > Alternatively is there some other intermediate step that people would prefer?
> > Similarly — maybe there's something I'm missing — maybe about differences between C/C++?
> >
> > Another option is to aim for implementing the functionality of __atomic_feraiseexcept inline in the fetch_add GCC 15 (a bit tight on time), to use it in the new fetch_add builtin overload, but not use it in the CAS loop.
> > That way no compiler & floating point type combination would need linking libatomic, current clang would not handle floating point exception information, while current gcc would handle that information.
> >
> > What are your thoughts?
>
> If making it Just Work without a new libatomic requirement is
> possible, I'd prefer that.
>
> > Regards,
> > Matthew
> >
> > ________________________________
> > From: Joseph Myers <josmyers@redhat.com>
> > Sent: 19 September 2024 10:38 PM
> > To: Matthew Malcomson <mmalcomson@nvidia.com>
> > Cc: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>; Jonathan Wakely <jwakely@redhat.com>; Richard Biener <rguenther@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 Sep 2024, mmalcomson@nvidia.com wrote:
> >
> > > 6) Anything special about floating point maths that I'm tripping up on?
> >
> > Correct atomic operations with floating-point operands should ensure that
> > exceptions raised exactly correspond to the operands for which the
> > operation succeeded, and not to the operands for any previous attempts
> > where the compare-exchange failed.  There is a lengthy note in the C
> > standard (in C11 it's a footnote in 6.5.16.2, in C17 it's a Note in
> > 6.5.16.2 and in C23 that subclause has become 6.5.17.3) that discusses
> > appropriate code sequences to achieve this.  In GCC the implementation of
> > this is in c-typeck.cc:build_atomic_assign, which in turn calls
> > targetm.atomic_assign_expand_fenv (note that we have the complication for
> > C of not introducing libm dependencies in code that only uses standard
> > language features and not <math.h>, <fenv.h> or <complex.h>, so direct use
> > of <fenv.h> functions is inappropriate here).
> >
> > I would expect such built-in functions to follow the same semantics for
> > floating-point exceptions as _Atomic compound assignment does.  (Note that
> > _Atomic compound assignment is more general in the allowed operands,
> > because compound assignment is a heterogeneous operation - for example,
> > the special floating-point logic in build_atomic_assign includes the case
> > where the LHS of the compound assignment is of atomic integer type but the
> > RHS is of floating type.  However, built-in functions allow memory orders
> > other than seq_cst to be used, whereas _Atomic compound assignment is
> > limited to the seq_cst case.)
> >
> > So it would seem appropriate for the implementation of such built-in
> > functions to make use of targetm.atomic_assign_expand_fenv for
> > floating-point environment handling, and for testcases to include tests
> > analogous to c11-atomic-exec-5.c that exceptions are being handled
> > correctly.
> >
> > Cf. N2329 which suggested such operations for C in <stdatomic.h> (but
> > tried to do to many things in one paper to be accepted into C); it didn't
> > go into the floating-point exceptions semantics but simple correctness
> > would indicate avoiding spurious exceptions from discarded computations.
> >
> > --
> > Joseph S. Myers
> > josmyers@redhat.com
> >
  
Matthew Malcomson Oct. 14, 2024, 5:06 p.m. UTC | #8
Just to double-check:  If C++ users don't really care about fenv, then:
Would a feasible shortcut be to add a #pragma to avoid caring about about this for a given region (and use that pragma in libstdc++ in order to avoid the regression of needing to link against libatomic).

The reason I'm looking to see what shortcuts are possible is that I'm quite dubious of implementing `__atomic_feraiseexcept` as a builtin correctly in the GCC 15 timeframe (on top of finishing off this patch).
Especially given that there seem to be two specialist implementations in backends I'm unfamiliar with.
  
Joseph Myers Oct. 14, 2024, 5:47 p.m. UTC | #9
On Mon, 14 Oct 2024, Matthew Malcomson wrote:

>   4. __atomic_feraiseexcept should be a builtin to avoid previously 
> unnecessary requirement to link libatomic.

libatomic should be linked by default (with --as-needed); see bug 81358.  
But if your concern is e.g. libstdc++.so having DT_NEEDED for libatomic, 
that might not suffice.

__atomic_feraiseexcept is a bit long to expand inline all the time (it's 
supposed to ensure that exceptions are raised including taking any enabled 
traps, so it's not just manipulating bits in the floating-point status 
register).  On the other hand, if you're only concerned with one 
operation, only a subset of the checks in __atomic_feraiseexcept are 
relevant (for example, addition and subtraction can't raise FE_DIVBYZERO; 
and except for the exact underflow case which __atomic_feraiseexcept 
doesn't handle anyway, they can't raise FE_UNDERFLOW).
  
Matthew Malcomson Oct. 15, 2024, 10:27 a.m. UTC | #10
Thanks for the pointer — always linking with libatomic by default using--as-needed sounds quite promising from my end.

I am not certain, but suspect that needing libatomic for atomic<float>::fetch_{add,sub} would not mean libstdc++.so would get a DT_NEEDED for libatomic.
The place where the new builtin would go is in a header that the user code would include (and would be inlined completely).
From a quick grep it appears this functionality in this header is not currently used in the libstdc++.so library itself.
Not 100% confident on that yet — but enough to feel this approach is promising.
I guess if that is the case, then using the same hypothetical driver flag suggested in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81358#c9 for linking libstdc++ seems like it could avoid adding the requirement without realising.

From reading the comments in that bugzilla it doesn't look like there's any objection to implementing that.
Seems that the testsuite and build system are the main things to watch out for here right?

Does that seem reasonable to others?
  
Joseph Myers Oct. 15, 2024, 6:10 p.m. UTC | #11
On Tue, 15 Oct 2024, Matthew Malcomson wrote:

> Seems that the testsuite and build system are the main things to watch 
> out for here right?

Yes, as illustrated by the draft patch linked from bug 81358 comment 11.