Skip to content

Add a few missing uses of types and enums to XML #960

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

Open
kpet wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add a few missing uses of types and enums to XML #960

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions xml/cl.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4383,6 +4383,19 @@ server's OpenCL/api-docs repository.
<type name="cl_image_format"/>
<type name="cl_buffer_region"/>
</require>
<require comment="API data types">
<type name="cl_char"/>
<type name="cl_uchar"/>
<type name="cl_short"/>
<type name="cl_ushort"/>
<type name="cl_int"/>
<type name="cl_uint"/>
<type name="cl_long"/>
<type name="cl_ulong"/>
<type name="cl_float"/>
<type name="cl_half"/>
<type name="cl_double"/>
</require>
<require comment="Constants">
<enum name="CL_CHAR_BIT"/>
<enum name="CL_CHAR_MAX"/>
Expand Down Expand Up @@ -4424,6 +4437,32 @@ server's OpenCL/api-docs repository.
<enum name="CL_HUGE_VAL"/>
<enum name="CL_MAXFLOAT"/>
<enum name="CL_INFINITY"/>
<enum name="CL_M_E"/>
<enum name="CL_M_LOG2E"/>
<enum name="CL_M_LOG10E"/>
<enum name="CL_M_LN2"/>
<enum name="CL_M_LN10"/>
<enum name="CL_M_PI"/>
<enum name="CL_M_PI_2"/>
<enum name="CL_M_PI_4"/>
<enum name="CL_M_1_PI"/>
<enum name="CL_M_2_PI"/>
<enum name="CL_M_2_SQRTPI"/>
<enum name="CL_M_SQRT2"/>
<enum name="CL_M_SQRT1_2"/>
<enum name="CL_M_E_F"/>
<enum name="CL_M_LOG2E_F"/>
<enum name="CL_M_LOG10E_F"/>
<enum name="CL_M_LN2_F"/>
<enum name="CL_M_LN10_F"/>
<enum name="CL_M_PI_F"/>
<enum name="CL_M_PI_2_F"/>
<enum name="CL_M_PI_4_F"/>
<enum name="CL_M_1_PI_F"/>
<enum name="CL_M_2_PI_F"/>
<enum name="CL_M_2_SQRTPI_F"/>
<enum name="CL_M_SQRT2_F"/>
<enum name="CL_M_SQRT1_2_F"/>
</require>
<require comment="Error codes">
<enum name="CL_SUCCESS"/>
Expand Down Expand Up @@ -5123,6 +5162,7 @@ server's OpenCL/api-docs repository.
<require>
<type name="cl_device_svm_capabilities"/>
<type name="cl_queue_properties"/>
<type name="cl_properties"/>
<type name="cl_svm_mem_flags"/>
<type name="cl_pipe_properties"/>
<type name="cl_pipe_info"/>
Expand Down Expand Up @@ -5589,6 +5629,18 @@ server's OpenCL/api-docs repository.
<require comment="cl_device_info">
<enum name="CL_DEVICE_HALF_FP_CONFIG"/>
</require>
<require comment="Constants">
<enum name="CL_HALF_DIG"/>
<enum name="CL_HALF_MANT_DIG"/>
<enum name="CL_HALF_MAX_10_EXP"/>
<enum name="CL_HALF_MAX_EXP"/>
<enum name="CL_HALF_MIN_10_EXP"/>
<enum name="CL_HALF_MIN_EXP"/>
<enum name="CL_HALF_RADIX"/>
<enum name="CL_HALF_MAX"/>
<enum name="CL_HALF_MIN"/>
<enum name="CL_HALF_EPSILON"/>
Comment on lines +5633 to +5642
Copy link
Contributor

@bashbaug bashbaug Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These half constants are also defined in cl_platform.h. Since these are #defines and not typedefs we could define them twice without a diagnostic (assuming they're defined to the same values, which hopefully they are!), though it might be better not to do this.

Do we want to:

  1. Consider these constants to be parts of the API, unconditionally, also, by including them as part of "OpenCL 1.0" and not part of the cl_khr_fp16 extension?
  2. Take them out of cl_platform.h and define them in cl_ext.h instead, as part of the cl_khr_fp16 extension?
  3. Some other solution that involves special-casing these constants in the extension header generation scripts.

Note, I originally thought it was weird that cl_half was in "OpenCL 1.0", especially because cl_double is not, but perhaps this is intended and correct due to the vload_half and vstore_half built-in functions?

Copy link
Contributor Author

@kpet kpet Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two identical copies of those definitions in cl_platform.h (MSVC/WIN32 and "others"). We probably ought to fix that too. I can't think of a strong reason for these not to be defined by cl_khr_fp16, maybe apart
from the fact that all applications will directly or indirectly include cl_platform.h but maybe not cl_ext.h. If we needed to have compiler-dependent variants of those definitions (such differences are typically handled in cl_platform.h) then presumably the issues would be generic and worthwhile to solve for all extensions in tooling. Let me outline the resolutions I see:

  1. Move the require tags to OpenCL 1.0 and accept that these are always available in OpenCL. They were clearly added by the cl_khr_fp16 spec so this would be a little weird.
  2. Move the definitions to their own enum block, require them in cl_khr_fp16 but keep the definitions in cl_platform.h.
  3. Move the definitions to their own enum block. require them in cl_khr_fp16 and rely on tooling to generate the definitions as part of cl_ext.h.

I think 1 does not look like a good option. 2 might require a special case in tooling but the headers wouldn't change. 3 looks like the more orthogonal but requires changing the headers.

Note, I originally thought it was weird that cl_half was in "OpenCL 1.0", especially because cl_double is not, but perhaps this is intended and correct due to the vload_half and vstore_half built-in functions?

I haven't done the full search though old references but, on the surface, that seems like a good enough reason.

Copy link
Contributor

@bashbaug bashbaug Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming: we decided to go with (2)? If so, this means we need to add a "denylist" to the header generation script for these enums. Not a big deal, but it's not something I'll do unless we need to do it.

</require>
</extension>
<extension name="cl_APPLE_SetMemObjectDestructor" revision="0.0.0" comment="not registered" supported="opencl">
<require>
Expand Down Expand Up @@ -5625,6 +5677,7 @@ server's OpenCL/api-docs repository.
<extension name="cl_loader_layers" revision="1.0.0" supported="opencl">
<require>
<type name="CL/cl_icd.h"/>
<type name="cl_icd_dispatch"/>
</require>
<require>
<type name="cl_layer_info"/>
Expand Down