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

Specification for cl_khr_extended_versioning #218

Merged
merged 3 commits into from
Nov 10, 2020

Conversation

kpet
Copy link
Contributor

@kpet kpet commented Feb 12, 2020

Signed-off-by: Kevin Petit kevin.petit@arm.com

@kpet
Copy link
Contributor Author

kpet commented Feb 12, 2020

Here's a first attempt at getting the specification for cl_khr_extended_versioning into the main extension specification. A few things I think we should discuss:

  • Individual extension specifications are currently grouped by the OpenCL version required. This means chapter numbers change as we insert extensions.
  • I've added a section with metadata about the extension. I'd like to do the same for all other extensions (at the very least add version information) but before I do that, I'd rather get a bit of feedback on the approach to reduce back and forth on the whole document.

@bashbaug
Copy link
Contributor

I have a few style opinions:

  • I think we should eliminate any notion of tying an extension to an OpenCL version and just add the extensions to the document in numerical order. The specification text has already eliminated this, I think, and the only reference to specific OpenCL versions is in the asciidoc source. This will keep chapter numbers consistent as extensions are added.

  • The only exception we may want to consider are the extensions to the SPIR-V environment, which we may want to either keep as the last chapter, or could consider moving to an Appendix.

  • I like some of the extension metadata, such as the version of the OpenCL specification it was written against, but some of the other metadata is debatably extraneous. A good example is the "issues", I think: this is useful information, but it is purely informational, so does it belong in the spec? As a compromise, could it be conditionally compiled into the spec (disabled by default)?

  • Could we move the "contributors" to the main spec acknowledgements vs. including it for each individual extension?

  • At least in the common case for ratified and final extensions, could we omit the "extension status"?

A few other comments:

  • Please add this extension to the "quick reference table" in quick_reference.asciidoc.

  • We purposefully haven't included definitions for the enum values for prior KHR extensions. I do see the value in including them, and we include them in vendor extensions, but if we're going to add them we should do so consistently.

In general though, this all looks very good. Thanks!

@kpet
Copy link
Contributor Author

kpet commented Apr 21, 2020

Picking this up again.

I think we should eliminate any notion of tying an extension to an OpenCL version and just add the extensions to the document in numerical order.

Agree, and this change has now been done in another PR.

[...] some of the other metadata is debatably extraneous. A good example is the "issues", I think: this is useful information, but it is purely informational, so does it belong in the spec? As a compromise, could it be conditionally compiled into the spec (disabled by default)?

I think it's useful info to answer the "what-were-they-thinking-when-they-designed-this type questions". I take your point though and I agree this may end up taking quite a bit of space in the spec. Given that we maintain the extension spec as a separate document, I'm not sure it's too big a deal. It seems to be working fine for Vulkan (these are in the appendix where extensions are described).

Could we move the "contributors" to the main spec acknowledgements vs. including it for each individual extension?

I disagree with this. I find it very useful to be able to know what companies/people were behind specific Vulkan extensions for example.

At least in the common case for ratified and final extensions, could we omit the "extension status"?

Yup, makes sense. I'll remove that.

Please add this extension to the "quick reference table" in quick_reference.asciidoc.

Will do.

We purposefully haven't included definitions for the enum values for prior KHR extensions. I do see the value in including them, and we include them in vendor extensions, but if we're going to add them we should do so consistently.

I think we should always do it. It's our best bet to guard against enum conflicts.

@kpet kpet force-pushed the extended-versioning-public branch from 7f4bd4e to 0c841b8 Compare April 21, 2020 14:53
@kpet kpet changed the title Preliminary specification for cl_khr_extended_versioning Specification for cl_khr_extended_versioning Apr 21, 2020
@kpet kpet force-pushed the extended-versioning-public branch from 0c841b8 to 4e80c99 Compare April 28, 2020 12:28
@kpet
Copy link
Contributor Author

kpet commented May 7, 2020

@bashbaug I think this is ready for another round of review when you have time.

@alycm alycm mentioned this pull request Aug 13, 2020
Copy link
Contributor

@alycm alycm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't come back to this sooner. One real open question here for me.

ext/cl_khr_extended_versioning.asciidoc Outdated Show resolved Hide resolved
ext/cl_khr_extended_versioning.asciidoc Show resolved Hide resolved
ext/cl_khr_extended_versioning.asciidoc Show resolved Hide resolved
ext/cl_khr_extended_versioning.asciidoc Show resolved Hide resolved
ext/cl_khr_extended_versioning.asciidoc Show resolved Hide resolved
@kpet
Copy link
Contributor Author

kpet commented Sep 7, 2020

@alycm Thanks for the review. I've rebased it and addressed your comments.

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
Change-Id: Ibe017baa7b8f0a24f9294aaabae4efeaadb43055
@kpet kpet force-pushed the extended-versioning-public branch from f39df02 to d3d0c32 Compare November 4, 2020 11:49
@kpet
Copy link
Contributor Author

kpet commented Nov 4, 2020

@alycm @bashbaug I've rebased this again. I would appreciate if we could get it out of the way :).

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please have one last look at some of the style comments I left previously - #218 (comment)?

Some of these are admittedly subjective, but this PR is doing things a bit inconsistently with the ways extensions have been documented in the past and I would like to be sure we are purposefully deviating.

I've also left a few specific comments inline. Thanks!

OpenCL_Ext.txt Outdated Show resolved Hide resolved

`cl_khr_extended_versioning`

==== Contributors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment regarding extension metadata like contributors, issues, etc. We haven't included this information in other KHR extensions. Is this a practice we want to continue into the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my position hasn't changed here and I think we should do that for all extensions.

ext/cl_khr_extended_versioning.asciidoc Outdated Show resolved Hide resolved
ext/quick_reference.asciidoc Outdated Show resolved Hide resolved
xml/cl.xml Show resolved Hide resolved
xml/cl.xml Show resolved Hide resolved
Change-Id: I44ef74894f3f0a77b226e4e123d3b917ec4518eb
Change-Id: If3aab0711f3645936bb74dec4c0fd32773551c1f
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, all my issues are resolved.

Let's see if we can merge this one in today's teleconference.

@bashbaug
Copy link
Contributor

Merging as discussed in the November 10th teleconference.

@bashbaug bashbaug merged commit bba7272 into KhronosGroup:master Nov 10, 2020
@kpet kpet deleted the extended-versioning-public branch January 6, 2021 09:16
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.

None yet

3 participants