CL 2.0 introduced atomics and generic address space so there were
only one set of APIs for doing atomics, however since CL 3.0
makes generic address space optional, there has to be new sets
of atomic interfaces to handle that cases.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Generally LGTM, but I think we should guard the else blocks with CL_VERSION_2_0 or higher to avoid exposing the functions in earlier standards because the spec says:
The C11 style atomic functions in this sub-section require support for OpenCL 2.0 or newer. However, this statement does not apply to the "OpenCL C 1.x Legacy Atomics" descriptions at the end of this sub-section.
Adding @azabaznov to check if he can suggest anything for testing. Although it seems like this is only related to the new functionality of OpenCL3.0.
clang/lib/Headers/opencl-c.h | ||
---|---|---|
13678 | can we annotate #endif with a comment, please to improve readability? The same applies to other places. |
The whole atomic section is wrapped in
#if defined(OPENCL_CPP_VERSION) || (OPENCL_C_VERSION >= CL_VERSION_2_0)
is that not good enough coverage?
I'll add the endif fixes.
clang/lib/Headers/opencl-c.h | ||
---|---|---|
13678 | Isn't this the same problem we talked about before? how to annotate #if defined(__opencl_c_generic_address_space) does it make sense to put //defined(__opencl_c_generic_address_space) on the endif? |
clang/lib/Headers/opencl-c.h | ||
---|---|---|
13304 | These new atomics with global and local pointer as argument were introduced only in OpenCL C 3.0 and have never been exposed in earlier versions, so I think you should guard them with #if (OPENCL_C_VERSION >= CL_VERSION_3_0) |
clang/lib/Headers/opencl-c.h | ||
---|---|---|
13678 | Yes indeed! |
clang/lib/Headers/opencl-c.h | ||
---|---|---|
13678 | with the change to add elif for OPENCL_C_VERSION these now get those comments instead of generic address space. |
clang/lib/Headers/opencl-c.h | ||
---|---|---|
13303 | Sorry, I overlooked that. Not elif, just if as these are available in 3.0 even with generic address space feature (https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#the-atomic_init-function): // Requires OpenCL C 3.0 or newer. void atomic_init(volatile __global A *obj, C value) void atomic_init(volatile __local A *obj, C value) // Requires OpenCL C 2.0, or OpenCL C 3.0 or newer and the // __opencl_c_generic_address_space feature. void atomic_init(volatile A *obj, C value) |
@airlied, can you please fix that to something like here https://godbolt.org/z/3Kbso8ca3? What currently we do have with your patch is that generic address space overload is always selected, but more strict overloading always exist in OpenCL C 3.0 (https://godbolt.org/z/vbMrdMxs1).
clang/lib/Headers/opencl-c.h | ||
---|---|---|
13379 | typo error on line 13379: |
Sorry, I overlooked that. Not elif, just if as these are available in 3.0 even with generic address space feature (https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#the-atomic_init-function):