Skip to content

CL_VERSION_* define does not match specification #282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
rjodinchr opened this issue Jun 2, 2025 · 6 comments · May be fixed by #283
Open

CL_VERSION_* define does not match specification #282

rjodinchr opened this issue Jun 2, 2025 · 6 comments · May be fixed by #283

Comments

@rjodinchr
Copy link

OpenCL-Headers/CL/cl_version.h

Lines 38 to 59 in 1ce4f1c

/* OpenCL Version */
#if CL_TARGET_OPENCL_VERSION >= 300 && !defined(CL_VERSION_3_0)
#define CL_VERSION_3_0 1
#endif
#if CL_TARGET_OPENCL_VERSION >= 220 && !defined(CL_VERSION_2_2)
#define CL_VERSION_2_2 1
#endif
#if CL_TARGET_OPENCL_VERSION >= 210 && !defined(CL_VERSION_2_1)
#define CL_VERSION_2_1 1
#endif
#if CL_TARGET_OPENCL_VERSION >= 200 && !defined(CL_VERSION_2_0)
#define CL_VERSION_2_0 1
#endif
#if CL_TARGET_OPENCL_VERSION >= 120 && !defined(CL_VERSION_1_2)
#define CL_VERSION_1_2 1
#endif
#if CL_TARGET_OPENCL_VERSION >= 110 && !defined(CL_VERSION_1_1)
#define CL_VERSION_1_1 1
#endif
#if CL_TARGET_OPENCL_VERSION >= 100 && !defined(CL_VERSION_1_0)
#define CL_VERSION_1_0 1
#endif

All CL_VERSION_* are defined by cl_version.h to 1 when not defined already. It does not match the specification:

CL_VERSION_1_0 Substitutes the integer 100 reflecting the OpenCL 1.0 version
https://github.com/KhronosGroup/OpenCL-Registry/blob/faccc50d088ff0028aed68e1ec3a2d468081a15c/sdk/3.0/docs/man/html/preprocessorDirectives.html#L838-L862
rjodinchr referenced this issue in llvm/llvm-project Jun 2, 2025
@frasercrmck
[libclc] Support the generic address space (#137183)
94142d9 
This commit provides definitions of builtins with the generic address
space.

One concept to consider is the difference between supporting the generic
address space from the user's perspective and the requirement for libclc
as a compiler implementation detail to define separate generic address
space builtins. In practice a target (like NVPTX) might notionally
support the generic address space, but it's mapped to the same LLVM
target address space as another address space (often the private one).

In such cases libclc must be careful not to define both private and
generic overloads of the same builtin. We track these two concepts
separately, and make the assumption that if the generic address space
does clash with another, it's with the private one. We track the
concepts separately because there are some builtins such as atomics that
are defined for the generic address space but not the private address
space.
rjodinchr added a commit to rjodinchr/OpenCL-Headers that referenced this issue Jun 2, 2025
@rjodinchr
fix CL_VERSION_* define
eb743fd 
According to the specification:
> CL_VERSION_1_0 Substitutes the integer 100 reflecting the OpenCL 1.0
version

Fix KhronosGroup#282
@rjodinchr rjodinchr linked a pull request Jun 2, 2025 that will close this issue
Open
@bashbaug
Copy link
Contributor

bashbaug commented Jun 2, 2025

To be clear, I think it's fine to "fix" this, and it'd probably avoid confusion, but I believe the spec text is only for OpenCL C CL_VERSION_* defines, because the referenced text is only in the OpenCL C spec and specifically references OpenCL C:

https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_C.html#preprocessor-directives-and-macros

@rjodinchr
Copy link
Author

Oh, so those are undefined in the spec (on the host side)?

@bashbaug
Copy link
Contributor

bashbaug commented Jun 2, 2025

Yeah, we currently don't say anything about these defines on the host, either in the OpenCL spec or even in this repo readme (debatably we should).

If you're able to say, do you have a use-case for the defines with specific integer versions? Or was this just something you noticed and were surprised by?

@rjodinchr
Copy link
Author

I don't have any issue with the current state. I was just surprised, and thought (wrongly) it should follow the OpenCL-C spec.

@rjodinchr
Copy link
Author

I think the most problematic part here is that those are getting exposed to anyone including the header, but they are not mentioned anywhere in the specification. Should they be undefined somewhere?

@bashbaug
Copy link
Contributor

bashbaug commented Jun 2, 2025

Cool, like I said previously, I'm fine merging your change to avoid even possible confusion. I've approved your PR. If there are no objections in tomorrow's teleconference let's just merge your PR.

As far as documentation goes, maybe we could just add a short paragraph to the README, at least for now?

https://github.com/KhronosGroup/OpenCL-Headers?tab=readme-ov-file#compiling-for-a-specific-opencl-version

Longer-term, we could consider adding some of this information in a spec appendix, similar to Vulkan:

https://registry.khronos.org/vulkan/specs/latest/html/vkspec.html#_vulkan_header_file_version_number

We could also undefine them somewhere, but IMHO they're useful, so it'd be better to keep them around and describe them instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix CL_VERSION_* define rjodinchr/OpenCL-Headers
2 participants
@bashbaug @rjodinchr