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

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

LGTM, modulo couple of nits.

@jyknight are you OK with this?

In D71726#2179428 <https://reviews.llvm.org/D71726#2179428>, @yaxunl wrote:

> Make IEEE single and double type as supported for fp atomics in all targets by default. This is based on the assumption that AtomicExpandPass or its ongoing work is sufficient to support fp atomics for all targets. This is to facilitate middle end and backend end development to support fp atomics.
>
> If a target would like to treat single and double fp atomics as unsupported, it can override the default behavior in its own TargetInfo.

Do we have sufficient test coverage on all platforms to make sure we're not generating something that LLVM can't handle everywhere?
If not, perhaps we should default to unsupported and only enable it for known working targets.



================
Comment at: clang/lib/CodeGen/CGAtomic.cpp:889-891
+    if (MemTy->isFloatingType()) {
+      ShouldCastToIntPtrTy = false;
+    }
----------------
`ShouldCastToIntPtrTy = !MemTy->isFloatingType();`


================
Comment at: clang/test/Sema/atomic-ops.c:102-103
 void f(_Atomic(int) *i, const _Atomic(int) *ci,
-       _Atomic(int*) *p, _Atomic(float) *d,
+       _Atomic(int*) *p, _Atomic(float) *d, _Atomic(double) *d2,
+       _Atomic(long double) *d3,
        int *I, const int *CI,
----------------
Rename arguments? d -> f, d2 -> d, d3 -> ld ?


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

https://reviews.llvm.org/D71726