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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

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.

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.


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

https://reviews.llvm.org/D71726