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

[Yaxun Liu via Phabricator via cfe-commits]

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 OpenCLMemoryScope {
+  WorkGroup = 1,
----------------
t-tye wrote:
> AtomicScopeOpenCLABI  to mirror AtomicOrderingCABI?
Will do.


================
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) &&
----------------
t-tye wrote:
> Should this take a LangOpt since different languages may use different value ABIs?
Although currently this function does not use LangOpt, but I agree it may be a good idea to make it future proof. Will add it and update comment.


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


https://reviews.llvm.org/D36580