[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Tony Tye via Phabricator via cfe-commits]

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,
----------------
AtomicScopeOpenCLABI  to mirror AtomicOrderingCABI?


================
Comment at: include/clang/Basic/SyncScope.h:94
+/// and return a language-specific value.
+inline bool isValidLanguageSyncScope(unsigned Scope) {
+  return Scope >= static_cast<unsigned>(OpenCLMemoryScope::WorkGroup) &&
----------------
Should this take a LangOpt since different languages may use different value ABIs?


================
Comment at: lib/CodeGen/CGAtomic.cpp:678
+  auto &Builder = CGF.Builder;
+  auto Scopes = getAllLanguageSyncScopes();
+  llvm::DenseMap<unsigned, llvm::BasicBlock *> BB;
----------------
Should only the scopes that apply to the language be returned otherwise will be generating code for invalid (possibly duplicate ABI) values?


================
Comment at: lib/Frontend/InitPreprocessor.cpp:582
   // The values should match clang SyncScope enum.
-  assert(static_cast<unsigned>(SyncScope::OpenCLWorkGroup) == 1 &&
-         static_cast<unsigned>(SyncScope::OpenCLDevice) == 2 &&
-         static_cast<unsigned>(SyncScope::OpenCLAllSVMDevices) == 3 &&
-         static_cast<unsigned>(SyncScope::OpenCLSubGroup) == 4);
+  assert(static_cast<unsigned>(OpenCLMemoryScope::WorkGroup) == 1 &&
+         static_cast<unsigned>(OpenCLMemoryScope::Device) == 2 &&
----------------
Could this be static_assert?


https://reviews.llvm.org/D36580