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

[Yaxun Liu via Phabricator via cfe-commits]

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

In D71726#2207148 <https://reviews.llvm.org/D71726#2207148>, @jyknight wrote:

> In D71726#2182667 <https://reviews.llvm.org/D71726#2182667>, @tra wrote:
>
>>> If a target would like to treat single and double fp atomics as unsupported, it can override the default behavior in its own TargetInfo.
>
> I really don't think this should be a target option at all. Every target can support the atomic fadd/fsub IR instruction (via lowering to a cmpxchg loop if nothing else). If it doesn't work, that's a bug in LLVM. We shouldn't be adding target hooks in Clang to workaround LLVM bugs, rather, we should fix them.
>
> There is one nit -- atomicrmw doesn't (yet) support specifying alignment. There's work now to fix that, but until that's submitted, only naturally-aligned atomicrmw instructions can be created. So, for now, supporting only a naturally-aligned floating-point add would be a reasonable temporary measure.

clang does not always emit atomic instructions for atomic builtins. Clang may emit lib calls for atomic builtins. Basically clang checks target info about max atomic inline width and if the desired atomic operation exceeds the supported atomic inline width, clang will emit lib calls for atomic builtins. The rationale is that the lib calls may be faster than the IR generated by the LLVM pass. This behavior has long existed and it also applies to fp atomics. I don't think emitting lib calls for atomic builtins is a bug. However, this does introduce the issue about whether the library functions for atomics are available for a specific target. As I said, only the target owners have the answer and therefore I introduced the target hook.

>> Do we have sufficient test coverage on all platforms to make sure we're not generating something that LLVM can't handle everywhere?



> Probably not.

In clang, we only test IR generation, as is done for other atomic builtins. fp atomics do not have less coverage compared with other atomic builtins. Actually for other atomic builtins we do not even test them on different targets. The ISA generation of fp atomics should be done in llvm tests and should not be blocking clang change.



================
Comment at: clang/lib/CodeGen/CGAtomic.cpp:937
+    if (Val1.isValid())
+      Val1 = Atomics.convertToAtomicIntPointer(Val1);
+    if (Val2.isValid())
----------------
jyknight wrote:
> convertToAtomicIntPointer does more than just cast to an int pointer, are you sure the rest is not necessary for fp types?
it is not needed for fp types. If the value type does not match the pointer type, clang automatically inserts proper llvm instructions to convert the value type to a value type that matches the pointer type. Two codegen tests are added (atomic_fetch_add(double*, float) and atomic_fetch_add(double*, int)) to test such situations. 


================
Comment at: clang/lib/Sema/SemaChecking.cpp:4837
         assert(Form != Load);
-        if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()))
+        if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()) ||
+            (IsAddSub && ValType->isFloatingType()))
----------------
jyknight wrote:
> This is confusing, and took me a bit to understand what you're doing. I'd suggest reordering the clauses, putting the pointer case first, e.g.:
> ```
> if (Form == Arithmetic && ValType->isPointerType())
>   Ty = Context.getPointerDiffType();
> else if (Form == Init || Form == Arithmetic)
>   Ty = ValType;
> else if (Form == Copy || Form == Xchg) .....
> else ......
> ...
> ```
done


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

https://reviews.llvm.org/D71726