Skip to content
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.

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

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

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

Conversation

kpet
Copy link
Contributor

@kpet kpet commented Aug 9, 2023

  • OpenCL 1.0 requires cl_char, etc types
  • OpenCL 1.2 and cl_khr+_fp64 require cl_double
  • cl_khr_fp16 requires CL_HALF_* constants
  • cl_khr_icd requires cl_icd_dispatch
  • OpenCL 1.0 requires all the CL_M_* constants. The specification does not state which version defines which constant (see CL_M_* constants unspecified #731)

Change-Id: I8eb34ab1eccf727700662ff5f61823d0e8c48ea1

@kpet
Copy link
Contributor Author

kpet commented Aug 9, 2023

With the change the only missing uses reported by #730 are vendor definitions and vendor-reserved enums.

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I have a few comments and suggestions, mostly from a header organization (and generation) point of view.

xml/cl.xml Outdated
@@ -5484,6 +5524,7 @@ server's OpenCL/api-docs repository.
<extension name="cl_khr_fp64" supported="opencl">
<require>
<type name="CL/cl.h"/>
<type name="cl_double"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is debatably the right thing to do, but cl_double is currently defined unconditionally in cl_platform.h. This means that the extension headers should never emit anything for cl_double, even if it matches what is currently in cl_platform.h, at least for C99:

https://stackoverflow.com/questions/26240370/why-are-typedef-identifiers-allowed-to-be-declared-multiple-times

So, what should we do about this? Some options:

  1. Consider cl_double to effectively be part of the API, unconditionally, meaning it's part of "OpenCL 1.0". This feels a little dirty, but I can't find any headers without cl_double so I think it's encoding reality.
  2. Consider cl_double to be an extension type only pre-OpenCL 1.2, and a core type for OpenCL 1.2 and newer. This is probably the "most correct", though I'm worried what this will break.
  3. Keep cl_double defined as-is in the headers and more-or-less as-defined here in the XML file, but add some sort of conditions or special-case code in the extension header generation scripts to avoid emitting anything for it.

(1) is the easiest thing to do. I wouldn't recommend (2). I think we could make (3) work but we'd need to figure out exactly what we want to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cl_double was described in the OpenCL 1.0 specification. In the interest of minimizing risk and not spending too much time on this, I suggest we go with 1, at least as a first step. I'll move the require tag for cl_double and we get forward progress.

Comment on lines +5541 to +5642
<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"/>
Copy link
Contributor

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

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.

xml/cl.xml Outdated
@@ -5518,6 +5571,7 @@ server's OpenCL/api-docs repository.
<extension name="cl_khr_icd" supported="opencl">
<require>
<type name="CL/cl.h"/>
<type name="cl_icd_dispatch"/>
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.

I'm not sure what to do with this one.

It doesn't seem right to include it with cl_khr_icd, since it doesn't define any APIs that take a cl_icd_dispatch or a pointer to a cl_icd_dispatch, and the extension text itself only describes struct _cl_icd_dispatch.

Maybe it should be part of cl_loader_layers, since it at least defines a few APIs that take a pointer to a cl_icd_dispatch?

If we consider this type to be a part of cl_loader_layers, though, should we take the typedef off of struct _cl_icd_dispatch though? Based on the current headers the type doesn't appear to be associated with any extension...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest we move the require tag to cl_loader_layers, at least as a first step. It's obviously the case and results at least one user for this which is the goal of this change.

xml/cl.xml Outdated Show resolved Hide resolved
- OpenCL 1.0 requires cl_char, etc types
- OpenCL 1.2 and cl_khr+_fp64 require cl_double
- cl_khr_fp16 requires CL_HALF_* constants
- cl_khr_icd requires cl_icd_dispatch
- OpenCL 1.0 requires all the CL_M_* constants. The specification does
  not state which version defines which constant (see KhronosGroup#731)

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
Change-Id: I8eb34ab1eccf727700662ff5f61823d0e8c48ea1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants