-
Notifications
You must be signed in to change notification settings - Fork 123
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.
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
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.
<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.
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.
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.
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 <[email protected]> Change-Id: I8eb34ab1eccf727700662ff5f61823d0e8c48ea1
260e5db
to
63371d5
Compare
Selection of non contentious changes from KhronosGroup#960. Also see KhronosGroup#1426
Change-Id: I8eb34ab1eccf727700662ff5f61823d0e8c48ea1