[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 3 inline comments as done.
yaxunl added a comment.

In D71726#2047566 <https://reviews.llvm.org/D71726#2047566>, @ldionne wrote:

> In D71726#1791904 <https://reviews.llvm.org/D71726#1791904>, @jfb wrote:
>
> > This generally seems fine. Does it work on most backends? I want to make sure it doesn't fail in backends :)
> >
> > Also, @ldionne / @EricWF / @mclow.lists do you need this in libc++ for floating-point atomic support?
>
>
> Yes, I guess we do in order to implement `fetch_add` & friends on floating point types (https://wg21.link/P0020R6).
>
> The builtins would need to work on `float`, `double` and `long double`. The code seems to suggest it does, however the tests only check for `float`. Does this support `__atomic_fetch_{add,sub}` on `double` and `long double`?


It depends on target. For x86_64, `__atomic_fetch_{add,sub}` on `double` and `long double` are translated to `__atomic_fetch_sub_8` and `__atomic_fetch_sub_16`.
For amdgcn, `__atomic_fetch_{add,sub}` on `double` is translated to fp atomic insts. `long double` is the same as `double` on amdgcn.



================
Comment at: clang/test/CodeGen/atomic-ops.c:296
+  // CHECK: fsub
+  return __atomic_sub_fetch(p, 1.0, memory_order_relaxed);
+}
----------------
ldionne wrote:
> Sorry if that's a dumb question, but I'm a bit confused: `p` is  a `float*`, but then we add a double `1.0` to it. Is that intended, or should that be `double *p` instead (or `1.0f`)?
In this case, the value type is converted to the pointee type of the pointer operand.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71726/new/

https://reviews.llvm.org/D71726