sameerds added inline comments.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13651
+ llvm::getConstantStringInfo(Scope, scp);
+ SSID = getLLVMContext().getOrInsertSyncScopeID(scp);
+
----------------
saiislam wrote:
> 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.
That's pretty strange. At this point, Clang should know what the target is, and
it should have a chance to update the list of sync scopes somewhere. @arsenm,
do you see a way around this?
================
Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9
+ // CHECK: fence syncscope("workgroup") seq_cst
+ __builtin_memory_fence(__ATOMIC_SEQ_CST, "workgroup");
+
----------------
JonChesterfield wrote:
> 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?
I think we agreed that this builtin exposes the LLVM fence exactly. That would
mean it takes arguments defined by LLVM. If you are implementing something
different from that, then it first needs to be specified properly. Perhaps you
could say that this is a C ABI compatible builtin, that happens to take target
specific scopes? That should cover OpenCL whose scope enum is designed to be
compatible with C.
Whatever it is that you are trying to implement here, it definitely does not
expose a raw LLVM fence.
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
