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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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

> In D71726#2537054 <https://reviews.llvm.org/D71726#2537054>, @tra wrote:
>
>> In D71726#2536966 <https://reviews.llvm.org/D71726#2536966>, @jyknight wrote:
>>
>>> My concern is that this is treating a backend _bug_ as if it were just an optional feature. But it's not the case that it might be reasonable to either implement or not implement this in a backend -- it should be implemented, and those that don't are buggy.
>>>
>>> I'd be happier with just having an ISEL failure when you try to use fp atomics on broken targets, rather than adding all this code and configuration to Clang in order to avoid that. (And, of course, the target maintainers should also fix them)
>>
>> +1. I agree with James.
>>
>> Removing code is often harder than adding it. When you're adding things, you're the only user. Once things are in, they will start growing dependencies that will need to be dealt with if you ever want to remove the code.
>>
>> Clean solution that works for AMDGPU only for now is better than a potentially permanent workaround.
>
> For amdgpu target, we do need diagnose unsupported atomics (not limited to fp atomics) since we do not support libcall due to ISA level linking not supported. This is something we cannot fix in a short time and we would rather diagnose it than confusing the users with missing symbols in lld.

Diagnosing that you don't support atomics your target can't reasonably support is completely fine.  (You could actually actually inline a locking approach if you really wanted to, though; Microsoft's `std::atomic` does that in the general case, although admittedly that's library code.)  I would like to understand whether that's really type-specific or just size-specific, though, and I don't think we've gotten a plain answer about that.  Is it true that amdgpu simply does not have a generic cmpxchg?

> For other targets, I can make changes to assume fp atomics are supported if width is within max inline atomic width of the target. Basically this will let fp atomics emitted for these targets and assuming middle end or backend will handle them properly.

I think that's reasonable.


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

https://reviews.llvm.org/D71726