-
Notifications
You must be signed in to change notification settings - Fork 115
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
Сonsistent enum types #926
base: main
Are you sure you want to change the base?
Conversation
Ah, also forgot to mention
|
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 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! |
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 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.
I mentioned that briefly in the PR description. But I also think the And on the side of other bindings, replacing 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. That said, separating changes to a separate PR is easy if you think it would still be better to do this in multiple steps.
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. |
I've split off changes to the Also added the new attributes to the schema, so this is now ready for review. |
Also, I noticed that here the enum group type was marked in a special way in the .asciidoc file when added to XML. |
@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? |
I think this is a merge-like issue related to #956. Can you try updating 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"/> |
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. |
Does it maybe make sense to create an issue like how you did for #798 to facilitate discussion? And the conflicts with my What can I do to help this move forward? |
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. |
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? |
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:
Maybe start with (4) and (5) and let's go from there? How important are (1), (2), and (3)? |
2935604
to
d32e04d
Compare
Remove `cl_device_avc_me_version` from `cl_intel_device_side_avc_motion_estimation`
Ok, I've fully removed everything you mentioned from (1) to (3), leaving only (4) and (5)
(1) is extremely important because it fixes a machine-readability problem with XML: (2) is similar, tho arguably less painful to fix on a per-binding basis. It's only passed into Basically, my main argument is that it's very easy to go from 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... The importance of (3) is... Well, so far, I thought of it as just minor fixes that are better to have than not.
Well, the
And it seems to be a convention to name the enum group types the same as the 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 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. Same for:
Do you have any objections/suggestions for adding enum types to these 4 extensions? Similar but not so clear cut:
On the other hand, this type is actually defined in the spec but inconsistent in XML:
I've removed all types above from this PR.
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? |
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. |
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 - 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.
<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"/> |
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 isn't correct either. CL_IMAGE_ROW_PITCH
and CL_IMAGE_SLICE_PITCH
are core enums are are not added by this extension.
<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"/> |
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.
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?
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 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?
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.
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.
Co-authored-by: Ben Ashbaugh <[email protected]>
I don't agree with that one thing about Your header generation script looks good. I should note that all
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. |
History
I first tried to rectify the idea of #779.
But there are enum types, like
cl_mem_flags
andcl_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: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: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 thecomment
attribute with something consistent:And also on using the defined enum types where they should be (like structs).
The
etype
is supposed to beenum
orbitfield
, whereenum
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"
andetype="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 ofcl_error_code
as a proper enum type, instead ofError 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.