sameerds 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:
> 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.
Right. It's actually an LLVM instruction, not even an intrinsic. I am guilty of
using the wrong term quite often, but usually the context helps. I think the
following is still needed:
# A testcase that exercises the builtin with an invalid string argument for
scope.
# An update to the change description describing what is being introduced
here. It is more than a direct mapping from builtin to instruction. The C ABI
is involved.
# An update to the Clang documentation describing this new builtin:
https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions
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
