Сonsistent enum types

[SunSerega]

History

I first tried to rectify the idea of #779.

But there are enum types, like cl_mem_flags and cl_svm_mem_flags, which have their value sets intersecting. I don't know how to describe that using <enums name="...", because <enum> tags are unique and sorted by value.

This is, however, already described in <require> tags:

        <require comment="cl_mem_flags and cl_svm_mem_flags - bitfield">

And after I've checked all of it, I can say - comment attributes actually more consistent. Even despite being a horrible abuse and a few cases like:

            <require condition="!defined(CL_VERSION_1_2)" comment="cl_device_info - defined in CL.h for OpenCL 1.2 and newer">

Making it hard to parse out the enum type name...
(this is what caused me to find #917)

---

This PR

So I've thrown away most changes #779 (my version of it) did to to <enums> tags for this PR and focused on replacing the comment attribute with something consistent:

        <require group="cl_device_type" etype="bitfield">

And also on using the defined enum types where they should be (like structs).

The etype is supposed to be enum or bitfield, where enum can be omitted, but can also be specified explicitly.
(I'll reflect this in registry.rnc, but would like to hear some feedback first)
I didn't find any cases where explicit etype="enum" would be useful, but I have some ideas concerning other changes I'd like to make further PRs for:

---

Other changes

Most notable of them is info from state tables, they are here (current commit) (compare).
But only after translating this from my internal binding fixers to etype="obj info" and etype="property list" did I think that there are much better ways to express this in XML. Well, I'd like to hear what others think before Iimit ideas to what I'm thinking of replacing it with.
Regardless, that branch is currently a mess, only good as proof of concept, because I'm currently using it for my bindings.

Other changes that probably should be in separate PR are the rename clCommandExecutionStatus=>cl_command_execution_status (for consistency of naming convention) and the addition of cl_error_code as a proper enum type, instead of Error codes comments.
But it felt awkward to separate because that would leave some stray comment attribute abuse cases in this PR.

Also, if anyone thinks there is value in how I merged main into #779 - it can be found here (current commit) (compare), in an even bigger mess of changes.

[SunSerega]

Ah, also forgot to mention etype="constants" and etype="OpenCL-C only".

• constants are intrinsic values, that are not actually grouped (and don't have their own enum type)

• Meanwhile OpenCL-C only is a hotfix, because I don't know what to do with them:
  Those enums are not actually part of OpenCL API. They only exist in kernels, but 2 intel motion estimation extensions added them to cl.xml...
  Probably don't want to lose this info, but also it doesn't seem consistent with other extensions.

[bashbaug]

Can you say a bit more how you expect these changes to be used? As-written they will require some updates to some of the tooling based on the XML file (the generated headers come to mind), but I don't mind making these updates if this will be useful.

Changing from cl_int to cl_error_code is going to be a pretty big change and is likely to be the most controversial, so if you can do without that one (or at least move it to a separate PR) that would probably help to get these changes in faster. Most of the others look to be XML cleanup that might affect tooling but not the actual API specification, though I haven't gone through everything in detail.

As an aside, it's unfortunate that #779 never got across the finish line; I think it was close and had a lot of nice improvements.

Thanks!

[SunSerega]

Can you say a bit more how you expect these changes to be used?

In high-level languages, it is a good practice to only let access enums through their type name. But for that, a consistent mapping (enum=>type) is needed.

Before, I primarily used info from <enums> blocks.
But <enums name="cl_device_info"> contains many enums from other types, so I then used info from <require> blocks to change mappings.
That wasn't a very good approach, because I only checked if the enum type name is one of the words in the comment attribute.
And so it still left many broken enum types, which I fixed using my own procedures.
For instance, the <require> block for cl_svm_mem_flags stole some enums from cl_mem_flags.
So I had to manually dup those enums, making sure they would be both in cl_mem_flags and cl_svm_mem_flags. Here are the instructions I made for my fixers:
https://github.com/SunSerega/POCGL/blob/27a1b7eb288d1822a8b95c0d301fc173b157139a/Packing/Template/OpenCL/Fixers/Enums/MissingDuplicates.dat

I'm currently looking at what info I can share by moving it to common XML, so others can use it without jumping through the same number of hoops.
And as a bonus, there are many chances for me to notice what I didn't fix, by using git diff everywhere I can, forcing myself to more consciously think about what changed between the old ways and new XML info.

Changing from cl_int to cl_error_code is going to be a pretty big change and is likely to be the most controversial, so if you can do without that one (or at least move it to a separate PR) that would probably help to get these changes in faster

I mentioned that briefly in the PR description.

But I also think the cl_int=>cl_error_code change is not as scary as it first looks.
On the side of C/C++, this is nothing but a visual change because of how typedef works.

And on the side of other bindings, replacing cl_error_code back with cl_int should be trivial. Or at least much less painful than replacing cl_int with cl_error_code - because of functions like clCreateProgramWithBinary with multiple error-returning parameters as well as functions like clSVMAlloc that do not return errors.

Then again, even if changing back is not trivial, the wonderful thing about using git here is that people can just stick to the previous commit, not pulling more changes until they can process them.
From my experience, even XML files of OpenGL, which is barely updated nowadays, tend to once in a while break my binding code-generators in the most unexpected ways. And it's only worse if the code consuming the XML just works with new changes because it likely means something wrong has happened and wasn't caught by any sanity checks.
So IMO being able to use older commits until ready and to manually check what changed and how it affects things - is already a must when using these XML files.
While on the good side - making change more in the face would result in more attention from XML consumers, so they would also think about how they can make use of the rest of the changes.

That said, separating changes to a separate PR is easy if you think it would still be better to do this in multiple steps.
I can also specifically move changes to <commands> to a separate PR while leaving <require group="cl_error_code"> in this PR.

Most of the others look to be XML cleanup that might affect tooling but not the actual API specification

Yep, I checked everything with the core and extension specs. Where it didn't make sense - I still added it to XML as it is in spec and made a note for myself to discuss it later by making issues like #932.

[SunSerega]

I've split off changes to the <commands> tag, among other reasons - it would allow me more concurrency in submitting PRs. Thought I would've liked a bit more of a discussion here...

Also added the new attributes to the schema, so this is now ready for review.

[SunSerega]

Also, I noticed that here the enum group type was marked in a special way in the .asciidoc file when added to XML.
Should I also do that for this PR?

[SunSerega]

@bashbaug can you please look at this PR again?

In particular, the test fails now, but it seems to have nothing to do with my changes. Maybe some of the recently merged PRs conflict with each other?

[bashbaug]

In particular, the test fails now, but it seems to have nothing to do with my changes. Maybe some of the recently merged PRs conflict with each other?

I think this is a merge-like issue related to #956. Can you try updating CL_DEVICE_HANDLE_LIST_KHR on line 7000? I think this will get past the build error:

diff --git a/xml/cl.xml b/xml/cl.xml
index 3bd3a24..8991c2b 100644
--- a/xml/cl.xml
+++ b/xml/cl.xml
@@ -6997,7 +6997,7 @@ server's OpenCL/api-docs repository.
                 <enum name="CL_SEMAPHORE_PROPERTIES_KHR"/>
                 <enum name="CL_SEMAPHORE_PAYLOAD_KHR"/>
                 <enum name="CL_SEMAPHORE_TYPE_KHR"/>
-                <enum name="CL_DEVICE_HANDLE_LIST_KHR"/>
+                <enum name="CL_SEMAPHORE_DEVICE_HANDLE_LIST_KHR"/>
             </require>
             <require group="cl_semaphore_properties_khr">
                 <enum name="CL_SEMAPHORE_TYPE_KHR"/>

[SunSerega]

Ah, you're right. I fixed this when merging into my custom branch (all my PRs and some more) and was sure I propagated it back, but apparently, I didn't.

[SunSerega]

Does it maybe make sense to create an issue like how you did for #798 to facilitate discussion?
I have other improvements I'd like to propose, which are blocked by this.

And the conflicts with my custom branch (containing everything I'd like to make PRs for, eventually), while manageable - appear very often.
Before trying to share my metadata with other XML consumers through this repo, I was just storing it in local files of my bindings' repo, and so I didn't have any conflicts. Only warnings when something not covered by my metadata appeared.
I moved to the custom branch to stress-test my changes on myself and ensure I didn't miss anything, but I didn't expect to be solving constant conflicts for almost a year...

What can I do to help this move forward?

[bashbaug]

Does it maybe make sense to create an issue like how you did for #798 to facilitate discussion?

Yes, I think this would be helpful, +1.

As for moving things forward, can this be broken into several smaller PRs rather than one big one? I'd imagine there are some changes that are less contentious that we could merge in the very near future, then we can focus on the changes that are more complicated.

[SunSerega]

Alright: #1145

As for moving things forward, can this be broken into several smaller PRs rather than one big one?

I already separated, what you asked me to, here.
I think the changes are pretty indivisible now.
But I'd separate it further if you have any suggestions.

[SunSerega]

Do you have any updates on this?

I've implemented all the requests/suggestions. Please let me know if you want me to change the issue or somehow split this PR even further.

And as I understand, to move forward, we need to mention people who might want to see this, right?

[bashbaug]

Hello, here are a few suggestions to break this PR up further. I don't think you necessarily need to do all of these, but I think a few of the changes are likely to be more contentious than others, and for those it'd be best to put them in a separate PR:

1. Addition of the cl_error_code type. Is this even needed right now? It doesn't seem to be used. Regardless, I think this is likely to be the most contentious. Note, some form of an error code "group" is probably fine, since this mostly seems like an XML convention.
2. Addition of new named types in the core specification, e.g. cl_command_execution_status (are there others?). I'm hesitant to add these to the XML file and the headers, certainly not without corrosponding spec updates.
3. Addition of new named types for extensions, e.g. cl_import_type_arm. I'm hesitant to add these to the XML file and the headers without extension spec updates, also. Note, assuming we decide to do this, these new types will also need proper suffixes, e.g. cl_context_property_diagnostics_level_intel rather than cl_context_property_diagnostics_level.
4. Updates to the XML schema for "etype" and "group". These generally seem reasonable and are not likely to be contentious.
   • Aside: If the "OpenCL-C only" enums really are specific to OpenCL C then I think the right course of action is to remove them from the XML file and the headers, since we don't currently include OpenCL C information in the XML file.
5. Any other XML fixes and corrections - should be non-contentious.

Maybe start with (4) and (5) and let's go from there?

How important are (1), (2), and (3)?

[SunSerega]

Ok, I've fully removed everything you mentioned from (1) to (3), leaving only (4) and (5)
It kinda sounds like you wanted me to also separate (4) and (5) into 2 new PRs, but that doesn't really make sense to me since they are dependent on each other...

---

How important are (1), (2), and (3)?

(1) is extremely important because it fixes a machine-readability problem with XML:
Currently, it's impossible to automatically determine which parameters/return values are meant to represent the error code.
Many functions don't return an error code in any way. clCreateProgramWithBinary returns error codes in 2 parameters at once.

(2) is similar, tho arguably less painful to fix on a per-binding basis. It's only passed into clSetUserEventStatus and clSetEventCallback, accepted as an argument to clSetEventCallback, and returned from clGetEventInfo. But there is nothing in XML to automatically find these 4 uses.

Basically, my main argument is that it's very easy to go from cl_error_code back to cl_int (1-to-1 type swap) but impossible to automatically go the other way, so it's more correct for common XML used by many bindings to have this separation.

But also relevant to this PR - (1) and (2) are both groups of enums, and their current definition in XML is the most awkward to parse of all groups...
So, to make all enum types consistent - we need a consistent type name for (1) and (2).

The importance of (3) is... Well, so far, I thought of it as just minor fixes that are better to have than not.
But I see that from your perspective, they are not minor, so let's split (3) even further:

---

3. Addition of new named types for extensions, e.g. cl_import_type_arm. I'm hesitant to add these to the XML file and the headers without extension spec updates, also.

Well, the cl_arm_import_memory spec says this:

      Valid <properties> are:

        * CL_IMPORT_TYPE_ARM
        * CL_IMPORT_TYPE_PROTECTED_ARM
        * CL_IMPORT_DMA_BUF_DATA_CONSISTENCY_WITH_HOST_ARM
        * CL_IMPORT_ANDROID_HARDWARE_BUFFER_PLANE_INDEX_ARM
        * CL_IMPORT_ANDROID_HARDWARE_BUFFER_LAYER_INDEX_ARM

      Valid values for CL_IMPORT_TYPE_ARM are:

        * CL_IMPORT_TYPE_HOST_ARM - this is the default
        * CL_IMPORT_TYPE_DMA_BUF_ARM
        * CL_IMPORT_TYPE_ANDROID_HARDWARE_BUFFER_ARM

And it seems to be a convention to name the enum group types the same as the _properties/_info enum they are bound to (So CL_IMPORT_TYPE_ARM => cl_import_type_arm)

And I think it's much better to always separate such groups of enums. Especially in the case of bitfields because they are the only items in the <properties> array that can be bit-combined.

This kind of info is necessary to generate fool-proofed high-lvl wrappers.

But I now see that the extension spec defines other enum types explicitly, yet for this one, there is only this mention I've shown above.
I guess the main problem here is a spec inconsistency. And it should be fixed separately from this PR...

Same for:

• cl_affinity_domain_ext from cl_ext_device_fission
• cl_device_me_version from cl_intel_advanced_motion_estimation
• cl_device_avc_me_version from cl_intel_device_side_avc_motion_estimation

Do you have any objections/suggestions for adding enum types to these 4 extensions?

Similar but not so clear cut:

• All fields of cl_motion_estimation_desc_intel struct from cl_intel_motion_estimation: Add types for fields from cl_motion_estimation_desc_intel #1261
• All fields of cl_mem_ext_host_ptr struct from cl_qcom_ext_host_ptr

On the other hand, this type is actually defined in the spec but inconsistent in XML:

• cl_context_property_diagnostics_level from cl_intel_driver_diagnostics: [XML] Fix type defined by cl_intel_driver_diagnostics #1260

I've removed all types above from this PR.
But the cl_command_termination_reason_arm type was already defined in spec and partially in XML, just not in <type> tags, so I kept it in this PR.

Aside: If the "OpenCL-C only" enums really are specific to OpenCL C then I think the right course of action is to remove them from the XML file and the headers, since we don't currently include OpenCL C information in the XML file.

Yes, I'm pretty sure I checked all of them when initially making this PR - and found that none are relevant outside of OpenCL-C.

I don't mind removing them from XML, but then we would be losing info. These enums are already neatly grouped, though I didn't find canonical names for those groups.

Generally, what do you think about eventually properly adding XML for OpenCL-C side info to this repo?
Maybe start with only the KHR stuff like cl_khr_subgroup_rotate...

[bashbaug]

Thanks for separating out the changes and explaining the rationale for (1), (2), and (3)!

Let's leave this more-or-less as-is for now, since it's already a pretty lengthy PR. Once we get this in we can start thinking about additional changes. Note: there is a merge conflict now, though I suspect it'll be fairly easy to resolve.

[bashbaug]

Thanks for doing this - there are a lot of nice updates here!

I updated the extension header generation scripts for these changes, which prompted most of these comments. If you're curious: KhronosGroup/OpenCL-Headers#263

It would be good to have somebody from Arm and perhaps Qualcomm review these changes also.

[bashbaug]

This isn't correct either. CL_IMAGE_ROW_PITCH and CL_IMAGE_SLICE_PITCH are core enums are are not added by this extension.

Suggested change

	<require group="cl_image_pitch_info_qcom">
	<enum name="CL_IMAGE_ROW_PITCH"/>
	<enum name="CL_IMAGE_ROW_ALIGNMENT_QCOM"/>
	<enum name="CL_IMAGE_SLICE_PITCH"/>
	<require group="cl_image_pitch_info_qcom">
	<enum name="CL_IMAGE_ROW_ALIGNMENT_QCOM"/>

[SunSerega]

Hmm, but these are newly added to cl_image_pitch_info_qcom otherwise:

Table 5.XXX

    List of supported param_names by clGetDeviceImageInfoQCOM

        cl_image_pitch_info_qcom       Return Type   Info returned in
                                                     param_value

        CL_IMAGE_ROW_PITCH             cl_uint       Returns the image row pitch
                                                     supported by this device

        CL_IMAGE_ROW_ALIGNMENT_QCOM    cl_uint       Returns the image row pitch
                                                     alignment supported by this
                                                     device

        CL_IMAGE_SLICE_PITCH           cl_uint       Returns the image slice
                                                     pitch supported by this
                                                     device

        CL_IMAGE_SLICE_ALIGNMENT_QCOM  cl_uint       Returns the image slice
                                                     pitch alignment supported
                                                     by this device

I think I already saw multiple extensions adding the same enums - in case either one of them is supported. Is it actually wrong for extension to add relevant enums again, even if they are in core?

[bashbaug]

I think we may have a different idea what should be included in the XML file. To be honest, I'm not sure if we have this written down anywhere (assuming my understanding is correct); maybe @oddhack can help.

My understanding is that the XML file is describing the types, enums, APIs, etc. that are added by a new extension, on top of whatever is supported in the core API. If two extensions happen to add the same new enum (or type, or API), and neither extension has a dependency on the other extension, that's OK and both extensions can require the same new enum, because extensions are intended to stand on their own. An extension would never require a core API enum explicitly though - instead, this requirement would be indicated by requiring a specific API version.

In the case above, cl_image_pitch_info_qcom can accept the CL_IMAGE_ROW_PITCH and CL_IMAGE_SLICE_PITCH enums, but these enums already exist in the core API and are not added by the extension. Therefore, I don't think they should be required by the extension, just like e.g. the cl_uint type is not required by the extension. Does this make sense?

[SunSerega]

Well, deciding if something should exist in XML without the context is indeed arbitrary.
But in this case, the reason for this info to exist in XML is the same as the reason for this PR: The consistent grouping of enums.

The extension adds functionality to these core enums (attached cl_uint return type, which I'm planning to cover in a future PR), and for that, also adds them to the cl_image_pitch_info_qcom enum group (which this PR's purpose covers). I don't know a better way to describe the fact that this extension adds 2 core enums to its new group.

Meanwhile, my argument about other extensions adding the same enum is more about the fact that we cannot break anything this way - since any code should already be capable of handling this.
Actually, a better example should be the cl_mem_flags and cl_svm_mem_flags groups, which add some of the same enums in the 1.0 version of the core in this PR.

[SunSerega]

I don't agree with that one thing about cl_image_pitch_info_qcom, but otherwise, all suggestions are done.

Your header generation script looks good. I should note that all <require> tags with enums inside are supposed to either have a group or etype (and I'm checking for that in my bindings), but I think that doesn't yet hold with this PR alone... Mainly because of error codes.

It would be good to have somebody from Arm and perhaps Qualcomm review these changes also.

Would you like to go ahead and ping them? I can't do this myself because I don't know who the relevant people are. But I'm fine waiting for a bit longer if the cause is quality.