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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/TargetInfo.h:1479
+    Unsupported,
+    Init,
+    C11LoadStore,
----------------
yaxunl wrote:
> rjmccall wrote:
> > `atomic_init` is not actually an atomic operation, so there's never an inherent reason it can't be supported.
> > 
> > In general, I am torn about this list, because it's simultaneously rather fine-grained while not seeming nearly fine-grained enough to be truly general.  What's actually going on on your target?  You have ISA support for doing some specific operations atomically, but not a general atomic compare-and-swap operation?  Which means that you then cannot support support other operations?
> > 
> > It is unfortunate that our layering prevents TargetInfo from simply being passed the appropriate expression.
> The target hook getAtomicSupport needs an argument for atomic operation. Since not all targets support fp add/sub, we need an enum for add/sub. Since certain release of iOS/macOS does not support C11 load/store, we need an enum for C11 load/store. We could define the enums as {AddSub, C11LoadStore, Other}. However, this would cause a difficulty for emitting diagnostic message for unsupported atomic operations since we map this enum to a string for the atomic operation and use it in the diagnostic message. 'Other' would be mapped to 'other atomic operation' which is not clear what it is.
It's not obviously true that not all targets support FP add/sub, though.  Any target that provides compare-and-swap at the width of an FP type can do an atomic FP add/sub at that width; it might be less efficient than it would be with specific ISA support, but that's true for a lot of atomic operations.  Surely it's better to just fix whatever bugs LLVM has with lowering atomic FP add/sub than to add more abstraction to Clang to handle a special case that shouldn't exist.

I don't know what issues Darwin has with C11 load/store; that might be a more compelling reason to have this abstraction, although again it seems strange that we're outlawing a specific operation when in principle we can just emit it less efficiently.


================
Comment at: clang/lib/Basic/Targets/AArch64.h:143
+    }
+  }
 };
----------------
yaxunl wrote:
> rjmccall wrote:
> > Why can't targets reliably expand this to an atomic compare-and-exchange if they support that for the target width?
> There are some bugs in either the middle end or backend causing this not working. For example, half type atomic fadd on amdgcn is not lowered to cmpxchg and the backend has isel failure, bf16 type atomic fadd on arm is not lowered to cmpxchg and the backend has isel failure. The support for each fp type needs to be done case by case. So far there is no target support atomic fadd/sub with half and bf16 type.
Are we legalizing atomicrmw to cmpxchg loops in the backend instead of as LLVM IR pass?  That seems like an architectural mistake.  Regardless, this bug should just be fixed.


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

https://reviews.llvm.org/D71726