kenba / cl3 Goto Github PK

The provisional command-buffer extension was recently added to the Khronos OpenCL-Headers repo, see KhronosGroup/OpenCL-Headers#182. This library should be updated to incorporate this change.

The Khronos OpenCL-Headers repo was recently changed, see KhronosGroup/OpenCL-Headers#184.
This library should be updated to incorporate this change.

Support for cl_khr_integer_dot_product v2 KhronosGroup/OpenCL-Headers#174 and cl_intel_device_attribute_query KhronosGroup/OpenCL-Headers#172 have recently been added to the Khronos OpenCL-Headers repo.
This library should be updated to incorporate these changes.

The Khronos OpenCL-Headers repo was recently changed, see KhronosGroup/OpenCL-Headers#197.
This library should be updated to incorporate this change.

The InfoType::to_string method causes the following clippy message:

error: type `info_type::InfoType` implements inherent method `to_string(&self) -> String` which shadows the implementation of `Display`
   --> src\info_type.rs:96:5
    |
96  | /     pub fn to_string(self) -> String {
97  | |         let mut a = self.to_vec_uchar();
98  | |
99  | |         // remove all trailing nulls, if any
...   |
105 | |         String::from_utf8_lossy(&a).into_owned()
106 | |     }
    | |_____^

As @vmx described in issue #4, it would be better to implement a From trait instead of to_string to fix this shadowing issue. However, care must be taken not to break issue #2.

External memory, external semaphore, and semaphore provisional extensions were recently added to the Khronos OpenCL-Headers repo, see KhronosGroup/OpenCL-Headers#180. This library should be updated to incorporate these changes.

@vmx has raised an issue (opencl3 #8) which is very similar to #1 raised by @jsatka.

There is clearly an issue with some OpenCL devices returning invalid CStrings in response to calling clGetProgramBuildInfo with CL_PROGRAM_BUILD_LOG and probably in other circumstances too.

The cl3 and opencl3 libraries should be resilient to such issues. Therefore, InfoType should be changed to make it easier to handle invalid CStrings from the OpenCL API in the opencl3 library.

The Khronos OpenCL-Headers repo was recently changed, see KhronosGroup/OpenCL-Headers#187.
This library should be updated to incorporate this change.

Add functions for kenba/opencl-sys-rs#3

The opencl-sys repo has been updated to reflect changes to the OpenCL-Headers repo.
The new functions should be added to this repo.

It is easier for users to handle standard Rust Strings than Rust ffi::CStrings.
Therefore, the API should use standard Rust Strings or strs instead of ffi::CStrings or ffi::CStr wherever possible.

Issue #6 shows that not all values for calls to functions such as clGetDeviceInfo are documented.
Also OpenCL extensions often add new values to such function calls.

The current implementation requires all param_names to functions such as clGetDeviceInfo to be known beforehand.
It would be more useful (and future proof) to provide functions to get data from functions such as clGetDeviceInfo for any param_name values, so that they can be called for new or undocumented values.

The change for issue #14 returns UUIDs or LUIDs for the relevant device param names.

The current implementation gets the values as a Vec<u8> then copies them into arrays of the appropriate size, i.e.: [u8; CL_UUID_SIZE_KHR] and [u8; CL_LUID_SIZE_KHR], so far so good.

The issue is that the library will panic if a device returns the wrong size for a UUID or LUID, either from the explicit assert or in the call to copy_from_slice.

This is wrong: the library shouldn't panic on bad data from a device, it should just return a Result error value.

The Khronos OpenCL-Headers repo was recently changed, see KhronosGroup/OpenCL-Headers#205.
This library should be updated to incorporate this change.

The Khronos OpenCL-Headers repo was recently changed, see KhronosGroup/OpenCL-Headers#193.
This library should be updated to incorporate this change.

I feel a bit bad about opening this issue as I proposed implementing From for the InfoType.

Looking at the current code, I think it could be more idiomatic Rust. When I proposed implementing From I thought that to_*() methods would be removed from InfoType as they are redundant (I'm a fan of having only a single way of doing things). Though I can understand to keep them for backwards compatibility.

Nonetheless, if we now have both, I propose replacing the From implementations with TryFrom. This way one could decide whether one wants to panic or process the error. Let me give you a code snipped to show what I mean.

#[derive(Debug)]
pub struct InfoTypeError(String);

impl TryFrom<InfoType> for cl_int {
    type Error = InfoTypeError;

    fn try_from(info_type: InfoType) -> Result<Self, Self::Error> {
        match info_type {
            InfoType::Int(a) => Ok(a),
            _ => Err(InfoTypeError("not a cl_int".to_string())),
        }
    }
}

impl InfoType {
    pub fn to_int(self) -> cl_int {
        cl_int::try_from(self).expect("not a cl_int")
    }
}

Another reason to use TryFrom is that the documentation for From states:

Note: This trait must not fail. If the conversion can fail, use TryFrom.

And I'd consider a panic also as "failing" (though, I'm not sure if it matches Rusts definition)

I need to send a clGetDeviceInfo which is not part of the standard, hence not part of the DeviceInfo enum. This means I cannot use the get_device_info() call.

What I currently do is using the api_info_value!() macro, it looks like this:

// Creates a new function called `get_device_info_bus_id` which takes two arguments, the device
// and the parameter ID.
api_info_value!(get_device_info_uint, cl_uint, clGetDeviceInfo);

pub fn get_nvidia_bus_id(d: &opencl3::device::Device) -> Result<u32, ClError> {
    const CL_DEVICE_PCI_BUS_ID_NV: u32 = 0x4008;
    let result = get_device_info_uint(d.id(), CL_DEVICE_PCI_BUS_ID_NV.into())?;
    Ok(result)
}

The macros to get values are used inside get_device_info() already. As they are useful outside of that function, I wondered if they could be made public. For example moving api_info_value!(get_value, cl_uint, clGetDeviceInfo); to the top-level of the file and making it public, e.g. as get_value_cl_uint().

This way my code wouldn't need to use a macro and could call it directly.

@vmx proposed creating a fork of cl-sys (cl3-sys?) in issue kenba/opencl3#30 in the opencl3 repo.
The rational is that cl-sys is no longer actively maintained.

This would involve making a new ffi repo, fixing bugs in the cl-sys repo, adding new OpenCL features and attempting to handle features properly between the new repo, this repo and the opencl3 repo.

Now would be a good time to do this since Rust is stable at Edition 2021 and the OpenCl SDK is also relatively stable.
However, I would appreciate Volke's help to design and debug the changes.

The pointer from an empty slice is not guaranteed to be null, as such all cl functions where the pointer has to null if the size is 0 can fail, because the api does not ensure that the pointer to empty list is null

The OpenCL release_* and retain_* functions alter the OpenCL C library's object reference counters, this can lead to undefined behaviour when called from the opencl3 library objects, see kenba/opencl3#51.

The svm_free function is also declared unsafe as it invalidates SVM pointers.

The CI I'm running on is using Debain Stretch as base system. There I get linker errors like this when I run cargo test.

/root/cl3/target/debug/deps/cl3-5580034d3b79b1c6.3heyk7dg5tpkvm6y.rcgu.o: In function `cl3::ext::set_mem_object_destructor_apple':
          /root/cl3/src/ext.rs:40: undefined reference to `clSetMemObjectDestructorAPPLE'
          collect2: error: ld returned 1 exit status

This was introduced recently by adding the extensions. It seems to be a problem with the environment. It works for me on two other, completely different systems (one of them is also Debian, though a newer version.

I even copied over the /usr/lib/x86_64-linux-gnu/libOpenCL.so from a machine where it works, to the machine where it doesn't. It's still the same error.

I'm still looking into this. I'm just opening this issue in hope that @kenba has seen this before and might have an idea.

OpenCL 3 can use the values: CL_DEVICE_UUID_KHR, CL_DRIVER_UUID_KHR and CL_DEVICE_LUID_KHR in calls to clGetDeviceInfo to get UUID and LUID values from OpenCL 3 devices. A UUID is a 16 byte array and a LUID is an 8 byte array.

Currently, UUIDs and LUIDs are both returned as Vec of the equivalent size and can be converted into Uuid an Luid types defined in device.rs. However, it would be more consistent if the Uuid an Luid types were values of the InfoType enum instead of newtypes in device.rs.

It would also be an opportunity to fix the bug in fmt::Display for Uuid!

The *_info functions take a *Info enum to get the relevant type of data for the given value.
Therefore the *_info functions can only handle known values; new OpenCL versions and extensions often provide new values to call: clGetPlatformInfo, clGetDeviceInfo, clGetProgramInfo, etc.

If the *_info functions just took a cl_platform_info, cl_device_info, cl_program_info, etc parameter instead, then the functions could return a Result<Vec, cl_int> for unknown but valid OpenCL values.

This would be a major change to the interface, but it would simplify the software and make it more resilient to OpenCL changes.

Ensure that deprecation notices are shown for the correct OpenCL versions as per issue kenba/opencl3#48 raised by @vmx.

As per @awused comments in kenba/opencl3#51 and kenba/opencl3#52, all OpenCL functions that can lead to undefined behaviour should be declared unsafe.

This issue is to capture and change all cl3 functions that should be declared unsafe.

Following on from issue #11. It is probably better (i.e. more idiomatic) to perform all the conversions from InfoType into Rust types using the From trait instead of the to_* methods.