JonChesterfield added inline comments.
================
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:
> sameerds wrote:
> > JonChesterfield wrote:
> > > sameerds wrote:
> > > > 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.
> > > The llvm fence, in text form, uses a symbol for the memory scope. Not an
> > > enum.
> > >
> > > This symbol is set using these macros for the existing atomic builtins.
> > > Using an implementation detail of clang instead is strictly worse, by
> > > layering and by precedent.
> > >
> > > ABI is not involved here. Nor is OpenCl.
> > The `__ATOMIC_*` symbols in Clang quite literally represent the C/C++ ABI.
> > See the details in AtomicOrdering.h and InitPreprocessor.cpp. I am not
> > opposed to specifying that the builtin expects these symbols, but then it
> > is incorrect to say that the builtin exposes the raw LLVM builtin. It is a
> > C-ABI-compatible builtin that happens to take target-specific scope as a
> > string argument. And that would also make it an overload of the already
> > existing builting __atomic_fence().
> I don't know what you mean by "raw", but am guessing you're asking for
> documentation for the intrinsic. Said documentation should indeed be added
> for this builtin - it'll probably be in a tablegen file.
I will try to stop using builtin and intrinsic as synonyms.
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
