This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] opencl-c.h: add CL 3.0 non-generic address space atomics
ClosedPublic

Authored by airlied on Jul 25 2021, 7:42 PM.

Details

Summary

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.

Diff Detail

Event Timeline

airlied created this revision.Jul 25 2021, 7:42 PM
airlied requested review of this revision.Jul 25 2021, 7:42 PM
airlied edited the summary of this revision. (Show Details)Jul 25 2021, 7:42 PM
airlied added a subscriber: azabaznov.

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.

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.

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.

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

Isn't this the same problem we talked about before? how to annotate

#if defined(__opencl_c_generic_address_space)
<decls>
#else
<alt decls>
#endif

does it make sense to put //defined(__opencl_c_generic_address_space) on the endif?

azabaznov added inline comments.Jul 28 2021, 3:02 AM
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)

Anastasia added inline comments.Jul 28 2021, 8:39 AM
clang/lib/Headers/opencl-c.h
13678

Yes indeed!

airlied updated this revision to Diff 362587.Jul 28 2021, 5:16 PM

added comments.

airlied added inline comments.Jul 28 2021, 5:18 PM
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.

Anastasia accepted this revision.Jul 29 2021, 3:46 AM

LGTM! Thanks

This revision is now accepted and ready to land.Jul 29 2021, 3:46 AM
azabaznov added inline comments.Jul 29 2021, 7:54 PM
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)
This revision was landed with ongoing or failed builds.Jul 29 2021, 9:47 PM
This revision was automatically updated to reflect the committed changes.

@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).

haonanya added inline comments.
clang/lib/Headers/opencl-c.h
13379

typo error on line 13379:
uint ovld atomic_fetch_xor(volatile local atomic_uint *object, uint operand);i