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

[John McCall via Phabricator via cfe-commits]

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 SyncScope {
----------------
If the numeric values here were chosen to align with the arguments to some runtime function, that's important to leave as a comment.


================
Comment at: lib/Headers/opencl-c.h:13145-13150
   memory_scope_work_item,
   memory_scope_work_group,
   memory_scope_device,
   memory_scope_all_svm_devices,
+#if defined(cl_intel_subgroups) || defined(cl_khr_subgroups)
   memory_scope_sub_group
----------------
yaxunl wrote:
> t-tye wrote:
> > Do these have to have the same values as the SycnScope enumeration? Should that be ensured in a similar way to the memory_order enumeration?
> It is desirable to have the same value as SyncScope enumeration, otherwise the library has to do the translation when calling __opencl_atomic_* builtins.
> 
> Will do.
Since we're defining these builtins ourselves de novo, it's fine to pick argument values that align with what the existing runtime functions expect.  Once the builtins are defined and in-use, of course, we cannot subsequently change the builtin values, even if the runtime changes.


https://reviews.llvm.org/D28691