-
Notifications
You must be signed in to change notification settings - Fork 116
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
base: main
Are you sure you want to change the base?
Conversation
With the change the only missing uses reported by #730 are vendor definitions and vendor-reserved enums. |
There was a problem hiding this 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"/> |
There was a problem hiding this comment.
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:
So, what should we do about this? Some options:
- 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 withoutcl_double
so I think it's encoding reality. - 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. - 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.
There was a problem hiding this comment.
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.
<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"/> |
There was a problem hiding this comment.
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:
- 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? - Take them out of
cl_platform.h
and define them incl_ext.h
instead, as part of thecl_khr_fp16
extension? - 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?
There was a problem hiding this comment.
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:
- 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. - Move the definitions to their own enum block, require them in
cl_khr_fp16
but keep the definitions incl_platform.h
. - Move the definitions to their own enum block. require them in
cl_khr_fp16
and rely on tooling to generate the definitions as part ofcl_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"/> |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
a0c0d7f
to
260e5db
Compare
- 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
260e5db
to
63371d5
Compare
Change-Id: I8eb34ab1eccf727700662ff5f61823d0e8c48ea1