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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to