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 types for fields from cl_motion_estimation_desc_intel #1261

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

Conversation

SunSerega
Copy link
Contributor

The relevant enum values are already grouped in the <require> tags.

I think it's better to allocate separate types for each such group, so it's possible for higher-lvl (than C) languages to enforce assigning the right values to fields of this struct.

Creating as a draft because the spec also needs to reflect this.

@bashbaug, this was a part of #926; what do you generally think about this change?
Any suggestions on how spec text should be handled?
I think it's not worth incrementing the version here since it wouldn't change the implementations in any way - it's purely an interface change.

P.S. If we agree on this, cl_mem_ext_host_ptr would also need a similar change. I'll hold off creating that PR for now.

SunSerega added a commit to SunSerega/OpenCL-Docs that referenced this pull request Sep 15, 2024
SunSerega added a commit to SunSerega/OpenCL-Docs that referenced this pull request Sep 15, 2024
@bashbaug
Copy link
Contributor

this was a part of #926; what do you generally think about this change?

I'm a little nervous about these changes. We probably should have done this from day one.... but we didn't.... and now this extension is more or less in maintenance (we don't support it on our latest GPUs), so I'm hesitant to make any changes unless we absolutely need to.

Is there something fundamentally broken using cl_uint vs. a named type, or is this a "nice-to-have"?

I do see that the values assigned to these cl_uint fields are incorrectly described as type="bitmask" in the XML file. They aren't bits in a bitmask, rather they're enumerated constants. If we were to fix this would it be sufficient?

@SunSerega
Copy link
Contributor Author

SunSerega commented Oct 13, 2024

Is there something fundamentally broken using cl_uint vs. a named type, or is this a "nice-to-have"?

This makes a big difference in high-level languages. Besides the default; being unable to set values to the wrong field - it allows generating a lot of convenient methods and operators.

But the main thing that's bugging me is:

            <require comment="cl_uint mb_block_type">

That's a human-readable construct - but not a machine-readable one.
It requires manual intervention to properly use when consuming XML.

If you think this is such an old extension that it should be ignored as a whole - we could mark that instead. I think it's less junky than what is currently in the <require comment="..."> for this extension. But still not great, and it might create a further divide in consistency as other extensions are updated.
And I see this extension should still exist in laptops that started releasing 7 ago (opencl.gpuinfo.org shows Intel(R) HD Graphics 630), so maybe it's not actually so old?

Personally, I think it's still the best course of action to create the types in XML and not touch the extension specs. They would be describing the exact same memory layout, but XML would be doing it in a way that I argue is better for XML consumers.

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