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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 7 inline comments as done.
yaxunl added inline comments.


================
Comment at: clang/include/clang/Basic/TargetInfo.h:1478
+  enum class AtomicOperationKind {
+    Unsupported,
+    Init,
----------------
rjmccall wrote:
> This shouldn't be here; if you have places that don't always represent an atomic operation, queries for the kind should return an `Optional<AtomicOperationKind>` from the classification.
Removed.


================
Comment at: clang/include/clang/Basic/TargetInfo.h:1479
+    Unsupported,
+    Init,
+    C11LoadStore,
----------------
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.


================
Comment at: clang/include/clang/Basic/TargetInfo.h:1497
+    Unsupported,
+  };
+
----------------
rjmccall wrote:
> I think this reflects our current strategies for emitting atomics, but it's a somewhat misleading enum in general because this isn't an exhaustive list of the options — there are certainly possible inline expansions that aren't lock-free.  (For example, you could have an inline spin-lock embedded in the atomic object.)  The goal of this enum is so that TargetInfo only has to have one hook for checking atomic operations?  I would be happier if you included an inline-but-not-lock-free alternative in this enum, even if it's never currently used, so that clients can do the right test.
Added InlineWithLock


================
Comment at: clang/include/clang/Basic/TargetInfo.h:1501
+  virtual AtomicSupportKind
+  getFPAtomicAddSubSupport(const llvm::fltSemantics &FS) const;
+
----------------
rjmccall wrote:
> Why is this needed as a separate hook?
Most target shares getAtomicSupport except FP atomic support, so define a virtual function for FP atomic support and let getAtomicSupport call it.


================
Comment at: clang/lib/AST/ASTContext.cpp:11046
+TargetInfo::AtomicOperationKind
+ASTContext::getTargetAtomicOp(const AtomicExpr *E) const {
+  switch (E->getOp()) {
----------------
rjmccall wrote:
> Should this be a method on `AtomicExpr`?  It seems like an intrinsic, target-independent property of the expression.
Yes. moved to AtomicExpr


================
Comment at: clang/lib/Basic/TargetInfo.cpp:870
+    return TargetInfo::AtomicSupportKind::Unsupported;
+  }
+  return AtomicWidthInBits <= AlignmentInBits &&
----------------
rjmccall wrote:
> Darwin targets should all be subclasses of `DarwinTargetInfo` in OSTargets.h, so you should be able to just override this there instead of having it in the base case.
done


================
Comment at: clang/lib/Basic/Targets/AArch64.h:143
+    }
+  }
 };
----------------
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.


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

https://reviews.llvm.org/D71726