yaxunl marked an inline comment as done.
yaxunl added inline comments.
Comment at: cfe/trunk/lib/Frontend/InitPreprocessor.cpp:581
// Define macros for the OpenCL memory scope.
// The values should match clang SyncScope enum.
+ static_assert(
t-tye wrote:
t-tye added inline comments.
Comment at: cfe/trunk/lib/Frontend/InitPreprocessor.cpp:581
// Define macros for the OpenCL memory scope.
// The values should match clang SyncScope enum.
+ static_assert(
// The values should match clang AtomicScopeOpenCLModel
This revision was automatically updated to reflect the committed changes.
Closed by commit rL310924: [OpenCL] Support variable memory scope in atomic
builtins (authored by yaxunl).
Changed prior to commit:
https://reviews.llvm.org/D36580?vs=60&id=85#toc
Repository:
rL LLVM
https://r
rjmccall accepted this revision.
rjmccall added a comment.
Thanks, that looks great.
https://reviews.llvm.org/D36580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
yaxunl updated this revision to Diff 60.
yaxunl marked 3 inline comments as done.
yaxunl added a comment.
Revised by John's comments.
https://reviews.llvm.org/D36580
Files:
include/clang/AST/Expr.h
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/SyncScope.h
lib/AST/Ex
yaxunl marked 6 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:
rjmccall wrote:
> yaxunl wrote
rjmccall added inline comments.
Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:
yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrot
yaxunl marked 4 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:
rjmccall wrote:
> yaxunl wrote
rjmccall added inline comments.
Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:
yaxunl wrote:
> rjmccall wrote:
> > Please correct me if I'm wrong, b
yaxunl marked 7 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/Basic/SyncScope.h:89
+ static std::unique_ptr create(LangOptions &Opt);
+};
+
rjmccall wrote:
> The previous design of this header seems more appropriate to me. Cla
rjmccall added inline comments.
Comment at: include/clang/Basic/SyncScope.h:59
+return "opencl_subgroup";
+ }
+}
yaxunl wrote:
> rjmccall wrote:
> > t-tye wrote:
> > > Should there be a default/assert/static_assert to allow SyncScope enum to
> > > grow due
t-tye accepted this revision.
t-tye added a comment.
LGTM
https://reviews.llvm.org/D36580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
yaxunl updated this revision to Diff 110904.
yaxunl marked 2 inline comments as done.
yaxunl added a comment.
Refactor to introduce classes AtomicScopeABI and AtomicScopeOpenCLABI for easy
extension to other languages.
https://reviews.llvm.org/D36580
Files:
include/clang/AST/ASTContext.h
i
yaxunl marked 6 inline comments as done.
yaxunl added inline comments.
Comment at: lib/CodeGen/CGAtomic.cpp:678
+ auto &Builder = CGF.Builder;
+ auto Scopes = getAllLanguageSyncScopes();
+ llvm::DenseMap BB;
t-tye wrote:
> yaxunl wrote:
> > t-tye wrote:
> > >
t-tye added inline comments.
Comment at: lib/CodeGen/CGAtomic.cpp:678
+ auto &Builder = CGF.Builder;
+ auto Scopes = getAllLanguageSyncScopes();
+ llvm::DenseMap BB;
yaxunl wrote:
> t-tye wrote:
> > Should only the scopes that apply to the language be returned
yaxunl marked 8 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/Basic/SyncScope.h:56
/// enums in opencl-c.h.
-enum class SyncScope {
- OpenCLWorkGroup = 1,
- OpenCLDevice = 2,
- OpenCLAllSVMDevices = 3,
- OpenCLSubGroup = 4,
+enum class Open
t-tye added inline comments.
Comment at: include/clang/Basic/SyncScope.h:56
/// enums in opencl-c.h.
-enum class SyncScope {
- OpenCLWorkGroup = 1,
- OpenCLDevice = 2,
- OpenCLAllSVMDevices = 3,
- OpenCLSubGroup = 4,
+enum class OpenCLMemoryScope {
+ WorkGroup = 1,
yaxunl updated this revision to Diff 110863.
yaxunl marked 5 inline comments as done.
yaxunl added a comment.
Revised by John's and Tony's comments.
https://reviews.llvm.org/D36580
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/SyncScope.h
lib/CodeGen/CGAtomic.cpp
yaxunl marked 10 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/Basic/SyncScope.h:46
+ Scopes.push_back(SyncScope::OpenCLSubGroup);
+ return Scopes;
+}
rjmccall wrote:
> t-tye wrote:
> > Should there be an assert/static_assert
rjmccall added inline comments.
Comment at: lib/CodeGen/CGAtomic.cpp:696
+if (S != Default)
+ SI->addCase(Builder.getInt32(static_cast(S)), B);
+
t-tye wrote:
> rjmccall wrote:
> > t-tye wrote:
> > > Is it documented in the SyncScope enum that the enumer
t-tye accepted this revision.
t-tye added a comment.
This revision is now accepted and ready to land.
LGTM other than suggested documentation and static_assert/unreachable comments.
Comment at: lib/CodeGen/CGAtomic.cpp:696
+if (S != Default)
+ SI->addCase(Builder.getIn
rjmccall added inline comments.
Comment at: include/clang/Basic/SyncScope.h:46
+ Scopes.push_back(SyncScope::OpenCLSubGroup);
+ return Scopes;
+}
t-tye wrote:
> Should there be an assert/static_assert in case SyncScope enum grows due to
> other languages?
It m
t-tye added inline comments.
Comment at: include/clang/Basic/SyncScope.h:46
+ Scopes.push_back(SyncScope::OpenCLSubGroup);
+ return Scopes;
+}
Should there be an assert/static_assert in case SyncScope enum grows due to
other languages?
Comme
rjmccall added inline comments.
Comment at: include/clang/Basic/SyncScope.h:47
+ return Scopes;
+}
+
You could just return an ArrayRef to a static const array.
Comment at: lib/CodeGen/CGAtomic.cpp:687
+
+ auto *SC = Builder.CreateIntCast(Scop
yaxunl created this revision.
https://reviews.llvm.org/D36580
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/SyncScope.h
lib/CodeGen/CGAtomic.cpp
lib/Sema/SemaChecking.cpp
test/CodeGenOpenCL/atomic-ops-libcall.cl
test/CodeGenOpenCL/atomic-ops.cl
test/SemaOpenC
25 matches
Mail list logo
