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

[Yaxun Liu via Phabricator via cfe-commits]

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<unsigned, llvm::BasicBlock *> BB;
----------------
t-tye wrote:
> yaxunl wrote:
> > 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.
> Curretly getAllLanguageSyncScopes does not take a LangOpt so not sure how it knows what the language is, and did not see it checking that the language is OpenCL internally. For non-OpenCL languages do they still have to support system scope?
For now I just assume all languages use OpenCL memory scope ABI. If a language has its own memory scope ABI it can be added later.


================
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 &&
----------------
t-tye wrote:
> Could this be static_assert?
Will do.


https://reviews.llvm.org/D36580