sameerds requested changes to this revision.
sameerds added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang/include/clang/Basic/Builtins.def:1583
+// Second argument : target specific sync scope string
+BUILTIN(__builtin_memory_fence, "vUicC*", "n")
+
----------------
This should be moved to be near line 786, where atomic builtins are listed
under the comment "// GCC does not support these, they are a Clang extension."
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13630
+
+ // Map C11/C++11 memory ordering to LLVM memory ordering
+ switch (static_cast<llvm::AtomicOrderingCABI>(ord)) {
----------------
There should no mention of any high-level language here. The correct enum to
validate against is llvm::AtomicOrdering from llvm/Support/AtomicOrdering.h,
and not the C ABI or any other language ABI.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13651
+ llvm::getConstantStringInfo(Scope, scp);
+ SSID = getLLVMContext().getOrInsertSyncScopeID(scp);
+
----------------
This seems to be creating a new ID for any arbitrary string passed as sync
scope. This should be validated against LLVMContext::getSyncScopeNames().
================
Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:5
+
+void test_memory_fence_success() {
+// CHECK-LABEL: test_memory_fence_success
----------------
There should be a line that tries to do:
__builtin_memory_fence(__ATOMIC_SEQ_CST, "foobar");
================
Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9
+ // CHECK: fence syncscope("workgroup") seq_cst
+ __builtin_memory_fence(__ATOMIC_SEQ_CST, "workgroup");
+
----------------
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
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
