JonChesterfield added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1888
+ // Check valididty of memory ordering as per C11 / C++11's memody model.
+ if (ord < static_cast<int>(llvm::AtomicOrderingCABI::acquire) ||
+ ord > static_cast<int>(llvm::AtomicOrderingCABI::seq_cst)) {
----------------
I think I'd write this as a switch over the enum instead of a ranged compare.
It'll codegen to the same thing, but we'll get warnings if more values are
introduced to the enum and things will keep working (here, anyway) if the
values are reordered.
================
Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9
+ // CHECK: fence syncscope("workgroup") seq_cst
+ __builtin_memory_fence(__ATOMIC_SEQ_CST, "workgroup");
+
----------------
saiislam wrote:
> sameerds wrote:
> > Orderings like `__ATOMIC_SEQ_CST` are defined for C/C++ memory models. They
> > should not be used with the new builtin because this new builtin does not
> > follow any specific language model. For user convenience, the right thing
> > to do is to introduce new tokens in the Clang preprocessor, similar to the
> > `__ATOMIC_*` tokens. The convenient shortcut is to just tell the user to
> > supply numerical values by looking at the LLVM source code.
> >
> > From llvm/Support/AtomicOrdering.h, note how the numerical value for
> > `__ATOMIC_SEQ_CST` is 5, but the numerical value for the LLVM
> > SequentiallyConsistent ordering is 7. The numerical value 5 refers to the
> > LLVM ordering "release". So, if the implementation were correct, this line
> > should result in the following unexpected LLVM IR:
> > fence syncscope("workgroup") release
> As you pointed out, the range of acquire to sequentiallly consistent memory
> orders for llvm::AtomicOrdering is [4, 7], while for llvm::AtomicOrderingCABI
> is [2, 5]. Enums of C ABI was taken to ensure easy of use for the users who
> are familiar with C/C++ standard memory model. It allows them to use macros
> like __ATOMIC_ACQUIRE etc.
> Clang CodeGen of the builtin internally maps C ABI ordering to llvm atomic
> ordering.
What language, implemented in clang, do you have in mind that reusing the
existing __ATOMIC_* macros would be incorrect for?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75917/new/
https://reviews.llvm.org/D75917
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
