[PATCH] D92004: [OpenCL] add CL 3.0 optional feature support to opencl-c.h

Fri Nov 27 06:37:03 PST 2020

svenvh added a comment.

> I am still unclear what should we do with testing though.

We probably don't want to hold up this patch until we have more thorough testing in place, since we don't even have any concrete plans for such testing at the moment.  Perhaps we should just land the patch once it's ready and if any problems are reported then we can always temporarily revert?

One idea for getting some confidence of not breaking OpenCL 2.0 too much, is to remove the `-fdeclare-opencl-builtins` flags from the SPIR-V LLVM translator <https://github.com/KhronosGroup/SPIRV-LLVM-Translator/> test suite and then run those tests to exercise `opencl-c.h` a bit.

================
Comment at: clang/lib/Headers/opencl-c-base.h:284
   memory_scope_work_group = __OPENCL_MEMORY_SCOPE_WORK_GROUP,
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0) || (__OPENCL_C_VERSION__ >= CL_VERSION_3_0 && defined(__opencl_c_atomic_scope_device))
   memory_scope_device = __OPENCL_MEMORY_SCOPE_DEVICE,
----------------
Anastasia wrote:
> I feel `__OPENCL_C_VERSION__ >= CL_VERSION_3_0` is redundant. Perhaps we don't need `__OPENCL_C_VERSION__ >= CL_VERSION_2_0` either if we define the feature macros above. Btw @azabaznov  is planning to define those macros in this header in https://reviews.llvm.org/D89869#change-kc4kXsHko6uZ. I guess some rebase will be necessary at some point depending on which commit will be ready first.
I also think it makes more sense to define the feature macros in `opencl-c-base.h`, unless there is a reason that I missed?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92004/new/

https://reviews.llvm.org/D92004