[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

This revision was automatically updated to reflect the committed changes. yaxunl marked 2 inline comments as done. Closed by commit rL310082: Add OpenCL 2.0 atomic builtin functions as Clang builtin (authored by yaxunl). Changed prior to commit: https://reviews.llvm.org/D28691?vs=109612&id=1097

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Still LGTM. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/lis

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl marked 3 inline comments as done. yaxunl added a comment. Ping. Any other comments? Thanks. Comment at: lib/AST/Expr.cpp:4000-4004 + if (auto AT = T->getAs()) { +return AT->getValueType(); + } else { +return T; + } bader wrote: > No need in el

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

bader accepted this revision. bader added inline comments. Comment at: lib/AST/Expr.cpp:4000-4004 + if (auto AT = T->getAs()) { +return AT->getValueType(); + } else { +return T; + } No need in else branch after return: ``` if (...) { return AT->getVa

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl marked 10 inline comments as done. yaxunl added inline comments. Comment at: include/clang/Basic/SyncScope.h:21 +/// \brief Defines the synch scope values used by the atomic builtins and +/// expressions +enum class SyncScope { rjmccall wrote: > If the num

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

t-tye accepted this revision. t-tye added a comment. LGTM https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl updated this revision to Diff 109612. yaxunl marked an inline comment as done. yaxunl added a comment. Added documentation about __OPENCL_MEMORY_SCOPE_* by Tony's comments. https://reviews.llvm.org/D28691 Files: docs/LanguageExtensions.rst include/clang/AST/Expr.h include/clang/Bas

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl marked an inline comment as done. yaxunl added inline comments. Comment at: docs/LanguageExtensions.rst:1935 +builtin function, and are named with a ``__opencl_`` prefix.) Low-level ARM exclusive memory builtins t-tye wrote: > Should it also say: > > `

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

t-tye added inline comments. Comment at: docs/LanguageExtensions.rst:1935 +builtin function, and are named with a ``__opencl_`` prefix.) Low-level ARM exclusive memory builtins Should it also say: ``` The macros ``__OPENCL_MEMORY_SCOPE_WORK_ITEM``, ``__OPENC

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl updated this revision to Diff 109591. yaxunl added a comment. Add comments to SyncScope.h https://reviews.llvm.org/D28691 Files: docs/LanguageExtensions.rst include/clang/AST/Expr.h include/clang/Basic/Builtins.def include/clang/Basic/DiagnosticSemaKinds.td include/clang/Basic/

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. LGTM. I'm fine with the plan to handle potentially non-constant scopes in a follow-up patch. Comment at: include/clang/Basic/SyncScope.h:21 +/// \brief Defines the synch scope values used by the atomic builtins and +/// expressions +enum class SyncSc

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl updated this revision to Diff 109556. yaxunl added a comment. Add assert to make sure pre-defined macros __OPENCL_MEMORY_SCOP_* to be consistent with SyncScope enum. https://reviews.llvm.org/D28691 Files: docs/LanguageExtensions.rst include/clang/AST/Expr.h include/clang/Basic/Bui

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl updated this revision to Diff 109542. yaxunl marked 5 inline comments as done. yaxunl added a comment. Herald added subscribers: aheejin, dschuff, jfb. Revised by Tony's and John's comments. https://reviews.llvm.org/D28691 Files: docs/LanguageExtensions.rst include/clang/AST/Expr.h

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl marked 6 inline comments as done. yaxunl added inline comments. Comment at: include/clang/Basic/Builtins.def:717 +ATOMIC_BUILTIN(__opencl_atomic_fetch_max, "v.", "t") + #undef ATOMIC_BUILTIN t-tye wrote: > Will the OpenCL 2.0 memory fences also be support

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. The patch generally looks good, but if you need to handle non-constant scopes, you should submit a new patch to address that. Comment at: lib/CodeGen/CGAtomic.cpp:896 +return V; + auto DestAS = getContext().getTargetAddressSpace(LangAS::o

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

t-tye added inline comments. Comment at: include/clang/Basic/SyncScope.h:23 +enum class SyncScope { + OpenCLWorkItem = 0, + OpenCLWorkGroup = 1, The OpenCL workitem scope is only used for image fences and does not apply to atomic operations so not sure that it

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl updated this revision to Diff 109406. yaxunl marked 29 inline comments as done. yaxunl added a comment. Revised by reviewers' comments. https://reviews.llvm.org/D28691 Files: docs/LanguageExtensions.rst include/clang/AST/Expr.h include/clang/Basic/Builtins.def include/clang/Basic

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:1 +//===--- SyncScope.h - atomic synchronization scopes *- C++ -*-===// +// Capitalization. Comment at: include/clang/Basic/SyncScope.h:20 + +namespace Syn

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. In https://reviews.llvm.org/D28691#823810, @Anastasia wrote: > In https://reviews.llvm.org/D28691#820684, @rjmccall wrote: > > > In https://reviews.llvm.org/D28691#820641, @b-sumner wrote: > > > > > In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > > > > > >

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

Anastasia added a comment. In https://reviews.llvm.org/D28691#820684, @rjmccall wrote: > In https://reviews.llvm.org/D28691#820641, @b-sumner wrote: > > > In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > > > > > In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > > > > > >

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

t-tye added inline comments. Comment at: include/clang/Basic/Builtins.def:717 +ATOMIC_BUILTIN(__opencl_atomic_fetch_max, "v.", "t") + #undef ATOMIC_BUILTIN Will the OpenCL 2.0 memory fences also be supported which also have a memory order and memory scope? ==

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. In https://reviews.llvm.org/D28691#820641, @b-sumner wrote: > In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > > > In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > > > > > There are other languages for heterogeneous compute that have scopes, >

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

b-sumner added a comment. In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > > > There are other languages for heterogeneous compute that have scopes, > > although not exposed quite as explicitly as OpenCL. For example AMD

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > There are other languages for heterogeneous compute that have scopes, > although not exposed quite as explicitly as OpenCL. For example AMD's "HC" > language. And any language making use of clang and targe

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

b-sumner added a comment. In https://reviews.llvm.org/D28691#820526, @rjmccall wrote: > In https://reviews.llvm.org/D28691#820489, @yaxunl wrote: > > > In https://reviews.llvm.org/D28691#820466, @b-sumner wrote: > > > > > Can we drop the "opencl" part of the name and use something like > > > __s

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. In https://reviews.llvm.org/D28691#820489, @yaxunl wrote: > In https://reviews.llvm.org/D28691#820466, @b-sumner wrote: > > > Can we drop the "opencl" part of the name and use something like > > __scoped_atomic_*? Also, it may not make sense to support non-constant >

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl added a comment. In https://reviews.llvm.org/D28691#820466, @b-sumner wrote: > Can we drop the "opencl" part of the name and use something like > __scoped_atomic_*? Also, it may not make sense to support non-constant > scope here since we can't predict what other scopes may be added by

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl updated this revision to Diff 108127. yaxunl added a comment. Add min/max and missing file. https://reviews.llvm.org/D28691 Files: docs/LanguageExtensions.rst include/clang/AST/Expr.h include/clang/Basic/Builtins.def include/clang/Basic/DiagnosticSemaKinds.td include/clang/Basi

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl marked an inline comment as done. yaxunl added a comment. In https://reviews.llvm.org/D28691#820375, @kzhuravl wrote: > Seems like SyncScope.h file is missing? Right. I will add it. https://reviews.llvm.org/D28691 ___ cfe-commits mailing l

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

b-sumner added a comment. Can we drop the "opencl" part of the name and use something like __scoped_atomic_*? Also, it may not make sense to support non-constant scope here since we can't predict what other scopes may be added by other languages in the future. https://reviews.llvm.org/D2869

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

kzhuravl added a comment. Seems like SyncScope.h file is missing? https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl marked 2 inline comments as done. yaxunl added inline comments. Comment at: include/clang/Basic/Builtins.def:713 +ATOMIC_BUILTIN(__opencl_atomic_fetch_or, "v.", "t") +ATOMIC_BUILTIN(__opencl_atomic_fetch_xor, "v.", "t") + b-sumner wrote: > yaxunl wrote: >

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

b-sumner added inline comments. Comment at: include/clang/Basic/Builtins.def:713 +ATOMIC_BUILTIN(__opencl_atomic_fetch_or, "v.", "t") +ATOMIC_BUILTIN(__opencl_atomic_fetch_xor, "v.", "t") + yaxunl wrote: > Anastasia wrote: > > What about min/max? I believe they w

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl updated this revision to Diff 108073. yaxunl marked 16 inline comments as done. yaxunl edited the summary of this revision. yaxunl added a reviewer: kzhuravl. yaxunl added a comment. Herald added a subscriber: nhaehnle. Rebased to ToT and revised by Anastasia's comments. https://reviews.l

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl marked 16 inline comments as done. yaxunl added inline comments. Comment at: include/clang/Basic/Builtins.def:713 +ATOMIC_BUILTIN(__opencl_atomic_fetch_or, "v.", "t") +ATOMIC_BUILTIN(__opencl_atomic_fetch_xor, "v.", "t") + Anastasia wrote: > What about min

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

Anastasia added inline comments. Comment at: include/clang/Basic/Builtins.def:713 +ATOMIC_BUILTIN(__opencl_atomic_fetch_or, "v.", "t") +ATOMIC_BUILTIN(__opencl_atomic_fetch_xor, "v.", "t") + What about min/max? I believe they will need to have the scope too. =

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl updated this revision to Diff 102374. yaxunl marked 11 inline comments as done. yaxunl edited the summary of this revision. yaxunl added a comment. Revised by John's comments. https://reviews.llvm.org/D28691 Files: docs/LanguageExtensions.rst include/clang/AST/Expr.h include/clang/

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. Looks like you haven't introduced proper constants in the header for scopes. Comment at: docs/LanguageExtensions.rst:1855 +atomic builtins are in ``explicit`` form of the corresponding OpenCL 2.0 +builtin function, and are named with a ``__opencl__`` p

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

yaxunl updated this revision to Diff 101955. yaxunl retitled this revision from "Support synchronisation scope in Clang atomic builtin functions" to "Add OpenCL 2.0 atomic builtin functions as Clang builtin". yaxunl edited the summary of this revision. yaxunl added a comment. Add __opencl_atomic