Skip to content

Comments

Add definition of cl_icdl extension.#886

Merged
bashbaug merged 6 commits intoKhronosGroup:mainfrom
Kerilk:cl_icdl
Mar 28, 2023
Merged

Add definition of cl_icdl extension.#886
bashbaug merged 6 commits intoKhronosGroup:mainfrom
Kerilk:cl_icdl

Conversation

@Kerilk
Copy link
Contributor

@Kerilk Kerilk commented Feb 24, 2023

This is the formal definition of the cl_icdl extension that adds info queries to an OpenCL ICD loader.
The PR for the headers is here:
KhronosGroup/OpenCL-Headers#214

Once those two are merged, we can update the loader and remove the temporary definition there.

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.

This mostly looks really good. I have a few questions though:

  1. Have you thought about how to publish this extension? Some possibilities:

    • Don't publish it at all, just put it in this repo as an asciidoc document.
    • Include it in the extension spec.
    • Build it as a standalone HTML doc and publish it via the registry like an EXT or vendor extension.
  2. Have you investigated using the generated asciidoctor dictionaries to help ensure a consistent look-and-feel in this extension spec? We probably wouldn't want to do this if we just left it as an asciidoc document since the GitHub rendering doesn't look as nice, but if we're intending to publish it as an HTML spec I'd strongly recommend at least looking into this.

@Kerilk
Copy link
Contributor Author

Kerilk commented Feb 27, 2023

To be completely honest, I am a bit lost. All the extensions I wrote have been just ASCII docs and I don't think they've been published. But, if we go the cl_ext route in the headers, maybe it makes sense to formally publish it. Would you have a preference?

@bashbaug
Copy link
Contributor

I'm totally open to alternatives but here's one possibility:

Treat this extension like any EXT or vendor extension. This means:

  1. Publish it on the OpenCL registry as a standalone HTML doc. Might require a small bit of refactoring to follow the format of other EXT and vendor extensions - the extension template should hopefully be useful.
  2. Include the definitions for this extension in cl_ext.h, like other extensions. Rationale: it's a user-facing extension just like any other, and this way we won't need to manage a new file.
  3. We'll need a good name for the extension since it will show up in the headers and have a link on the registry even if it doesn't show up in any queryable extension strings. cl_loader_info, similar to cl_loader_layers? cl_icd_loader_info? cl_icdl_info?

Maybe we can discuss this as an open in tomorrow's teleconference?

@Kerilk
Copy link
Contributor Author

Kerilk commented Mar 1, 2023

@bashbaug Thanks for the valuable input. I gave it a shot, what do you think?
I still need to investigate the asciidoctors dictionaries.

@Kerilk
Copy link
Contributor Author

Kerilk commented Mar 1, 2023

Tagging @vdanjean as he is credited in the extension author list.

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.

LGTM!

I checked and the header generation scripts "just worked" for this extension, pretty cool.

I'll leave this open for a couple more days just in case there are other comments, otherwise I'll merge first thing next week.

@bashbaug
Copy link
Contributor

Merging as discussed in the March 28th teleconference.

@bashbaug bashbaug merged commit d1aaa33 into KhronosGroup:main Mar 28, 2023
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