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

OpenCL Header only Dynamically Opened ICD Loader Proposal #169

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Kerilk
Copy link
Contributor

@Kerilk Kerilk commented May 16, 2021

This is a (full fledged, functional) straw-man proposal of an implementation of one of @bashbaug ideas for easing OpenCL ICD loader issues. These headers allow an OpenCL application to be built normally or with the -DOPENCL_LOAD flag active. In this case, linking with the loader is not necessary anymore as it is opened dynamically.
The title is slightly misleading as ONE source file from the project must also include the CL/opencl_load_lib.h to have a single definitions of the global variables used. (on windows this could go away as on MSVC inline functions can declare non const static variables).

This allows:

  • Functions failing with an error code if the loader cannot be found
  • Functions failing with another error code if the loader does not export the required function
  • Using outdated loader while still using the loader for dispatch, layers are working
  • It should be able to load a non khr_icd driver

This does not allow an application to:

  • Use recent OpenCL functions on an outdated loader: function will return an error
  • Avoid segfaulting when carelessly calling newer functionalities from an outdated implementation

The last point could be fixed using indirect dispatch in the loader.
I will add CI tests for it.

@bashbaug
Copy link
Contributor

Cool! This looks like a mix between the previous Static Lib with Dynamic Loader and ICD Loader Header File proposals. Having working code to play around with and demonstrate that it works is awesome.

Out of curiousity are you generating these files, or did you author them manually? I think it should be possible to generate most of them.

I think we mostly need to figure out which solution (or solutions, this doesn't need to be one-size-fits-all) we want and then start adding them to the SDK / CI / etc. There are clearly lots of different options, with different pros and cons.

Note that there are some aspects of this PR that we may want to adopt regardless, for example it looks like we both added some sort of a define to control linkage in the header file - see CLLD_INLINE in this PR and CL_API_LINKAGE in my proposal.

@Kerilk
Copy link
Contributor Author

Kerilk commented May 18, 2021

Cool! This looks like a mix between the previous Static Lib with Dynamic Loader and ICD Loader Header File proposals. Having working code to play around with and demonstrate that it works is awesome.

Thanks, I didn't knew about those (only about your presentation, I didn't find pointer to code in it), I will need to study them, they look great.

Out of curiousity are you generating these files, or did you author them manually? I think it should be possible to generate most of them.

I did not generate them yet because I wanted to do a full pass on the headers to be sure not to miss something. There are some strange inconsistencies:

  • Some APIS/extensions not behind version guards: if we generate the headers we may end up adding all the correct guards and maybe break some codes. Do we want to do this?
  • Windows extensions not having a function declaration but only function pointers, whereas all other loader extensions have function declarations. It broke my assumption that extensions functions present in the loader were declared in the headers (to be able to link with the loader), while other had function pointers types declarations. This led me to extract the function definitions from the cl_icd.h file to avoid forcing the inclusion of directx headers/extensions when compiling for windows, as I assumed without declaration nobody ever linked with those directly. It also led to my remark in Code Generation for Extension Headers #161 (comment).
  • We need to decide if we want to keep the deprecation prefixes/suffixes on function definitions, especially if they are inline. The function typdefs from Code Generation for Extension Headers #161 (comment) would help handling this more cleanly by I am unsure they will appeal to everyone.

I think we mostly need to figure out which solution (or solutions, this doesn't need to be one-size-fits-all) we want and then start adding them to the SDK / CI / etc. There are clearly lots of different options, with different pros and cons.

Yes, hopefully this one will help accomplish this goal. For this work there is some thread safety involved, so maybe using it to run the conformance test may be better (though I don't know how much parallel testing the conformance tests do). We also need some way to be able to install a working driver on windows image in GitHub actions. Ubuntu is easy, MacOS can work (with reservations), but for now I don't have a solution for windows...

Note that there are some aspects of this PR that we may want to adopt regardless, for example it looks like we both added some sort of a define to control linkage in the header file - see CLLD_INLINE in this PR and CL_API_LINKAGE in my proposal.

I agree with you completely. I started with a slighly more complex proposal initially that allowed even more flexibility, by having non static inline functions with extern declarations in the library. And you could switch between static inline/inline mode. This may be desirable and could allow to adapt the scheme to more exotic platforms.

CL/opencl_load_lib.h Outdated Show resolved Hide resolved
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.

3 participants