[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:1478
+  enum class AtomicOperationKind {
+    Unsupported,
+    Init,
----------------
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.


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


================
Comment at: clang/include/clang/Basic/TargetInfo.h:1497
+    Unsupported,
+  };
+
----------------
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.


================
Comment at: clang/include/clang/Basic/TargetInfo.h:1501
+  virtual AtomicSupportKind
+  getFPAtomicAddSubSupport(const llvm::fltSemantics &FS) const;
+
----------------
Why is this needed as a separate hook?


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


================
Comment at: clang/lib/Basic/TargetInfo.cpp:870
+    return TargetInfo::AtomicSupportKind::Unsupported;
+  }
+  return AtomicWidthInBits <= AlignmentInBits &&
----------------
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.


================
Comment at: clang/lib/Basic/Targets/AArch64.h:143
+    }
+  }
 };
----------------
Why can't targets reliably expand this to an atomic compare-and-exchange if they support that for the target width?


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

https://reviews.llvm.org/D71726