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

Begin integration of EXT extensions into the unified specification #1213

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

kpet
Copy link
Contributor

@kpet kpet commented Jul 17, 2024

  • Add a khr+ext spec type to makeSpec and cover in CI
  • Document the version and dependencies of all EXT extensions in the XML
  • Integrate cl_ext_cxx_for_opencl into the unified specification
  • Add placeholder descriptions for all EXT extensions linking to either core/KHR features that supersede old extensions that were never part of the specification or the OpenCL Extensions document.

This enables us to integrate EXT extensions incrementally and integrate future EXT extensions directly into the unified specification.

Change-Id: Ic634ce000ad3ebfb56e56bce91f9c0de3e786383

- Add a khr+ext spec type to makeSpec and cover in CI
- Document the version and dependencies of all EXT extensions in the XML
- Integrate cl_ext_cxx_for_opencl into the unified specification
- Add placeholder descriptions for all EXT extensions linking to either
  core/KHR features that supersede old extensions that were never
  part of the specification or the OpenCL Extensions document.

This enables us to integrate EXT extensions incrementally and integrate
future EXT extensions directly into the unified specification.

Change-Id: Ic634ce000ad3ebfb56e56bce91f9c0de3e786383
Signed-off-by: Kevin Petit <kevin.petit@arm.com>
@kpet kpet requested a review from bashbaug July 17, 2024 12:48
Co-authored-by: Ewan Crawford <ewan.cr@gmail.com>
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

LGTM

After building this my main thought would be whether it's worth splitting the appendix of supported extensions into separate KHR and EXT lists, as the single list is already quite long (although it is ordered to make the EXTs easy to find in that list)


=== Description

This extension is described in the OpenCL Extensions document.
Copy link
Contributor

Choose a reason for hiding this comment

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

This extension is described in the OpenCL Extensions document.

Providing a link here (and in similar descriptions) would be helpful to the reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree but it's hard to provide a link that's valid regardless of the spec you're looking at. Links would be different for published PDF or HTML versions of the spec and different again for the dev versions. I felt this was a bit too hard for a "temporary" (tm) problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool, we can leave this out of it isn't trivial.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note, the EXT extensions actually aren't currently in the OpenCL Extensions document, and they're documented in standalone extension docs on the registry, similar to vendor extensions. For example:

https://registry.khronos.org/OpenCL/extensions/ext/cl_ext_float_atomics.html

Until this changes, maybe it is possible to put a stable link to the appropriate extension document?

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 was looking at the PDF when writing this. The spec tooling generates:

  • out/pdf/OpenCL_Ext.pdf titled "The OpenCL Extension Specification"
  • out/pdf/extensions.pdf titled "OpenCL Extensions"

It's the latter that this refers to and that one does include EXT extensions. I agree there is potential for confusion and the real fix IMO is to get rid of both these documents :).

We could do what you propose but this would always link to the latest pulished version of the specification in HTML form. It would likely be useful to a good number of users but would at the same time potentially point to the wrong document for some others.

Copy link
Contributor

Choose a reason for hiding this comment

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

The tooling generates "extensions.pdf", but AFAIK we don't publish it. We still do publish "OpenCL_Ext.pdf", but we might not do so for long - see #1111.

What's the disadvantage of linking to the HTML spec? The way I look at it, if we don't put any link, readers looking for more information about the extension are either going to get frustrated and give up, or they're going to search for the name of the extension, which will (hopefully!) point them to the HTML spec on the registry. If that's the case, why not give them the link directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beyond having PDFs link to the HTML specs, the main issue I have with adding these links is that the version linked would not be the one aligned to the document linking to it. It's not an issue for published versions of the specification but if I package a set of PDFs generated from an arbitrary SHA for example, the link would potentially point to the wrong spec text. This mostly affects IHVs so maybe not a good reason to deprive OpenCL users of a helpful link even if only temporary so. I'll add the links with text stating that they point to the "latest published" version of the specifications.

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.

Thank you for doing this!

api/cl_ext_migrate_memobject.asciidoc Outdated Show resolved Hide resolved
Change-Id: I13b4860dfcd3d6d865b269847c5876bf75516e87
Change-Id: Ifddbbc47ddb0ac9be6327d9925682b96829d0946
@kpet
Copy link
Contributor Author

kpet commented Aug 13, 2024

@bashbaug I think I've addressed all your comments, PTAL. Note that we need to merge KhronosGroup/OpenCL-Registry#156 too (seems we never published that one on the registry; can't wait for this to become unnecessary :))

@bashbaug
Copy link
Contributor

Merging as discussed in the August 13th teleconference.

@bashbaug bashbaug merged commit a26e00b into KhronosGroup:main Aug 13, 2024
2 checks passed
@@ -6764,7 +6764,7 @@ server's OpenCL/api-docs repository.
<enum name="CL_QUEUE_COMPUTE_UNIT_LIMIT_ARM"/>
</require>
</extension>
<extension name="cl_ext_cxx_for_opencl" revision="0.0.0" supported="opencl">
<extension name="cl_ext_cxx_for_opencl" revision="1.0.0" depends="CL_VERSION_3_0,CL_VERSION_2_0+cl_khr_extended_versioning" supported="opencl">
Copy link
Contributor

@SunSerega SunSerega Aug 13, 2024

Choose a reason for hiding this comment

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

Can you please explain what it means, to have 2 different versions of core in the depends attribute?

Given coma, IG it means this extension can be supported if either:

  • OpenCL is 2.0 and cl_khr_extended_versioning is supported.
  • OpenCL is 3.0 is supported.

This is my best guess, but this is the first time coma was used in depends and the explanation in registry.rnc is still pretty surface-lvl:

# Boolean expression of core version and extension names using (),+ operators
Depends = attribute depends { text }

Before I thought the coma here is not an operator but a separator between () and + operators...


Also below:

        <extension name="cl_ext_float_atomics" revision="1.0.0" depends="CL_VERSION_2_0,CL_VERSION_3_0" supported="opencl">

Wouldn't CL_VERSION_3_0 here just be redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given coma, IG it means this extension can be supported if either: [...]

You got it!

Wouldn't CL_VERSION_3_0 here just be redundant?

I don't think so. OpenCL 3.0 is not a strict superset of OpenCL 2.0. The dependency is on "OpenCL C 2.0 atomic syntax" (according to https://registry.khronos.org/OpenCL/extensions/ext/cl_ext_float_atomics.html) which is available in the 2.x versions and 3.0. (I'm sure we could improve the tooling to express better dependencies but given where things are I think it's the best way of representing the dependency).

@kpet kpet deleted the integrate-ext branch August 14, 2024 10:41
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.

4 participants