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 dynamic runtime (declined) #33

Closed
wants to merge 24 commits into from

Conversation

dmitry-zakablukov
Copy link
Contributor

This pr adds a feature dynamic-runtime to load the OpenCL library in the runtime and avoid linkage errors during compilation stage (OpenCL.lib not found).

Additional changes:

  • all functions of cl3 are safe now
  • all constants of runtime are re-exported via cl3::constants module
  • all types of runtime are re-exported via cl3::types module

Constants and types refactoring was done because there was a possibility to import CL_SUCCESS constant, for example, from several modules of cl3. It is still an issue thow for a static runtime, but with dynamic runtime there is only the one module of cl3 from which a constant CL_SUCCESS can be imported.

I have checked that there is no compilation warnings with both runtimes and with default and full features sets.

Test are passing using the command cargo test --all-features -- --test-threads=1.

@kenba
Copy link
Owner

kenba commented Nov 12, 2024

Thank you @dmitry-zakablukov, you have clearly put a lot of effort into this.

Unfortunately, from my perspective, you've done too much! This PR:

  • is too big;
  • incorporates additional changes other than just fixing the dynamic runtime issue;
  • depends on another opencl sys crate: opencl-dynamic-sys;
  • and breaks the opencl3 crate.

I do not have the time to review all of your changes. I appreciate that you want to restructure the library and ensure that all the functions are safe. However, I would prefer those issues to be handled in separate merge requests.

Please can you remove all the extra changes and just provide the minimum changes necessary to fix the dynamic runtime issue. Ideally with the new functions in your opencl-dynamic-sys crate incorporated into the opencl-sys-rs crate and the dependency on opencl-dynamic-sys removed.

@dmitry-zakablukov
Copy link
Contributor Author

@kenba , I understand. Unfortunately, the main problem with opencl-sys-rs crate is that it requires the static linkage with OpenCL.lib during the compilation stage.

Consider the following code:

fn main() {
    println!("{}", opencl_sys::CL_SUCCESS);
}

It can not be built on some machines (my included) because of an error:

  = note: LINK : fatal error LNK1181: cannot open input file 'OpenCL.lib'

Note, that there is no any function calls. It is just a constant imported from the crate. Nevertheless, the code will still not compile.

So, if I make changes to opencl-sys-rs crate to support dynamic case, it will be even bigger PR than this PR (in my opinion), because I will need to remove all existing code of the crate and replace it with the dynamic one. I think, it is easier to support two different crates.

Secondly, opencl-sys-rs is no-std crate. And opencl-dynamic-sys cannot be made no-std because of its nature. So, I don't want to change that for opencl-sys-rs crate.

incorporates additional changes other than just fixing the dynamic runtime issue;

These changes is a small fraction from the whole PR. What I can do is:

  1. Return unsafe functions.
  2. Return the structure of exports from the cl3 crate.

But I really do not like the situation when the imports can be made from several places of cl3 crate.

and breaks the opencl3 crate.

I am ready to support the dynamic case in opencl3 crate as well.

================

Let me start with unsafe functions in a separate PR.

@dmitry-zakablukov
Copy link
Contributor Author

unsafe fn removed: #34

@dmitry-zakablukov dmitry-zakablukov changed the title OpenCL dynamic runtime OpenCL dynamic runtime (declined) Nov 15, 2024
@dmitry-zakablukov
Copy link
Contributor Author

Declining PR because of too much dirty commits, a new PR will be opened soon.

@dmitry-zakablukov dmitry-zakablukov deleted the dynamic-runtime branch November 15, 2024 09:40
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