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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
