saiislam marked 5 inline comments as done.
saiislam added inline comments.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13630
+
+ // Map C11/C++11 memory ordering to LLVM memory ordering
+ switch (static_cast<llvm::AtomicOrderingCABI>(ord)) {
----------------
sameerds wrote:
> 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.
Even though this builtin is supposed to be language-independent, here intention
was to provide interface in terms of well known standard C11/C++11 enums for
memory order (__ATOMIC_ACQUIRE, etc.), so that user of the builtin don't have
to remember and modify their code. The builtin internally maps it as per the
expectation of fence instruction.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13651
+ llvm::getConstantStringInfo(Scope, scp);
+ SSID = getLLVMContext().getOrInsertSyncScopeID(scp);
+
----------------
sameerds wrote:
> This seems to be creating a new ID for any arbitrary string passed as sync
> scope. This should be validated against LLVMContext::getSyncScopeNames().
As the FE is not aware about specific target and implementation of sync scope
for that target, getSyncScopeNames() here returns llvm'sdefault sync scopes,
which only supports singlethreaded and system as valid scopes. Validity
checking of memory scope string is being intentionally left for the later
stages which deal with the generated IR.
================
Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9
+ // CHECK: fence syncscope("workgroup") seq_cst
+ __builtin_memory_fence(__ATOMIC_SEQ_CST, "workgroup");
+
----------------
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.
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
