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");
+
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75917/new/
https://reviews.llvm.org/D75917
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
