Switch from ocl to opencl3 #34

vmx · 2021-05-18T14:02:48Z

This PR makes the switch from ocl to opencl3. This is mostly what the first commit. On top of that there are other improvements which make sense in the light of future CUDA integration.

For easier review you might want to review this PR commit per commit (also see the commit messages for more information).

This PR also needs changes to the upstream dependants, which can be found here:

src/opencl/error.rs

Cargo.toml

src/opencl/mod.rs

dignifiedquire · 2021-05-20T14:44:49Z

src/opencl/mod.rs

+            let kernels = opencl3::kernel::create_program_kernels(&program)?;
+            let kernels_by_name = kernels
+                .into_iter()
+                .map(|kernel| (kernel.function_name().unwrap(), kernel))


why is it safe to unwrap here?

src/opencl/mod.rs

vmx · 2021-05-20T16:59:03Z

I've converted this to a draft PR. It turns out that this work is not needed for the the UUID work at #30, hence I get more time to polish it more, like removing unwraps() and getting features like Debug implementations implemented upstrea. Thanks for the early review @dignifiedquire.

vmx · 2021-05-31T14:02:00Z

I have addressed previous comments and also cleaned-up the API a bit. It's ready for review :)

src/opencl/mod.rs

dignifiedquire · 2021-06-01T11:14:00Z

src/opencl/mod.rs

+        F: FnOnce() -> Result<R, E>,
+        E: From<GPUError>,
+    {
+        fun()


so much fun

src/opencl/mod.rs

dignifiedquire

a couple small things, but otherwise lgtm

vmx · 2021-06-07T14:01:57Z

I've removed the last commit (I'm not sure if that change is possible, once there is CUDA as well). Better be safe than sorry.

It can be merged if the corresponding neptune and bellperson changes are also merged.

vmx · 2021-06-14T16:51:09Z

I see failures in neptune. Not sure if it's really related, I'll have another look before merging.

vmx · 2021-07-05T17:04:00Z

@dignifiedquire I think nothing changed since the last review, is it OK if I merge it?

dignifiedquire

can you make sure to add doc comments please to all new functions

src/opencl/error.rs

dignifiedquire · 2021-07-06T10:46:37Z

src/opencl/mod.rs

-        Ok(())
-    }
+    buffer: opencl3::memory::Buffer<T>,
+    length: usize,


please add a comment if the length is in in bytes or in T elements

dignifiedquire · 2021-07-06T10:48:21Z

src/opencl/mod.rs

+            let binaries = program
+                .get_binaries()
+                .map_err(|_| GPUError::ProgramInfoNotAvailable(CL_PROGRAM_BINARIES))?;
+            std::fs::write(cached, binaries[0].clone())?;


what if there are multiple or no binary?

No idea. This code was just ported over from: https://github.com/filecoin-project/rust-gpu-tools/pull/34/files/e212d9d1af6d46c0030c2794dc0607939c031ff2#diff-f16a498f7eb37b8af9a85fcbd3bba8f972198602f61b6f919bf15aeb939da2c2L256

Should this be addressed? If yes, then I'd do this in a separate PR after all these reviews.

lets open an issue, to at least investigate this

dignifiedquire · 2021-07-06T10:49:12Z

src/opencl/mod.rs

+        let buff = opencl3::memory::Buffer::create(
+            &self.context,
+            CL_MEM_READ_WRITE,
+            length,


is length now in numbers of T?

Yes, length is the number of Ts. Reason is that we don't use as underlying buffer an OpenCL <u8> buffer anymore, but a <T> one (see https://github.com/filecoin-project/rust-gpu-tools/pull/34/files/e212d9d1af6d46c0030c2794dc0607939c031ff2#diff-f16a498f7eb37b8af9a85fcbd3bba8f972198602f61b6f919bf15aeb939da2c2R56). This simplifies the code quite, we need to do less size calculations and memory transformations. The tests still pass.

dignifiedquire · 2021-07-06T10:49:56Z

src/opencl/mod.rs

+    pub fn write_from_buffer<T>(
+        &self,
+        buffer: &Buffer<T>,
+        offset: usize,


what unit does offset have?

dignifiedquire · 2021-07-06T10:50:25Z

src/opencl/mod.rs

+        offset: usize,
+        data: &[T],
+    ) -> GPUResult<()> {
+        assert!(offset + data.len() <= buffer.length);


A useful error message would be nice

dignifiedquire · 2021-07-06T10:50:45Z

src/opencl/mod.rs

+            .create_sub_buffer(CL_MEM_READ_WRITE, offset, data.len())?;
+
+        self.queue
+            .enqueue_write_buffer(&buff, CL_BLOCKING, 0, data, &[])?;


maybe add a comment why passing an empty slice is the right thing to do here

I was about to add // We are not waiting for events, hence the last parameter is an empty slice, but that didn't really make things clearer. Also it would need to be added to all 3 occurrences. I'm in favour of not having a comment, but if you think I should that comment I can add it.

Also refactor the code. Instead of operating on the `Buffer`s, operate on the `Program` and pass in the `Buffer`s. This way there is no need to expose the queue to the outside. Kernel creation might now return an error if the kernel with the given name doesn't exist.

This change is needed to make the API the same that is used by the CUDA implementation.

This method is needed by the CUDA implementation, on OpenCL it doesn't really do anything other then executing the closure.

There is no need to keep a copy of a device, we only need the device name.

In CUDA it is non-optional, hence make it also mandatory for OpenCL.

There is no need to own it.

Remove `unwrap()` with proper error propagation.

There are functions/macros that are not used by `bellperson` or `neptune`. Remove the unused ones or make them private if they are used internally.

Instead of using a generic type in our `Buffer` type and do manual size calculations pass it on directly into opencl3.

vmx

Would it be OK if I add documentation to all public functions once we've merged everything (before the release)?

vmx · 2021-07-06T11:32:56Z

src/opencl/mod.rs

+            let binaries = program
+                .get_binaries()
+                .map_err(|_| GPUError::ProgramInfoNotAvailable(CL_PROGRAM_BINARIES))?;
+            std::fs::write(cached, binaries[0].clone())?;


No idea. This code was just ported over from: https://github.com/filecoin-project/rust-gpu-tools/pull/34/files/e212d9d1af6d46c0030c2794dc0607939c031ff2#diff-f16a498f7eb37b8af9a85fcbd3bba8f972198602f61b6f919bf15aeb939da2c2L256

Should this be addressed? If yes, then I'd do this in a separate PR after all these reviews.

vmx · 2021-07-06T11:38:41Z

src/opencl/mod.rs

+        let buff = opencl3::memory::Buffer::create(
+            &self.context,
+            CL_MEM_READ_WRITE,
+            length,


Yes, length is the number of Ts. Reason is that we don't use as underlying buffer an OpenCL <u8> buffer anymore, but a <T> one (see https://github.com/filecoin-project/rust-gpu-tools/pull/34/files/e212d9d1af6d46c0030c2794dc0607939c031ff2#diff-f16a498f7eb37b8af9a85fcbd3bba8f972198602f61b6f919bf15aeb939da2c2R56). This simplifies the code quite, we need to do less size calculations and memory transformations. The tests still pass.

vmx · 2021-07-06T12:07:35Z

src/opencl/mod.rs

+            .create_sub_buffer(CL_MEM_READ_WRITE, offset, data.len())?;
+
+        self.queue
+            .enqueue_write_buffer(&buff, CL_BLOCKING, 0, data, &[])?;


I was about to add // We are not waiting for events, hence the last parameter is an empty slice, but that didn't really make things clearer. Also it would need to be added to all 3 occurrences. I'm in favour of not having a comment, but if you think I should that comment I can add it.

dignifiedquire · 2021-07-06T12:37:23Z

Would it be OK if I add documentation to all public functions once we've merged everything (before the release)?

Sure, just open an issue to ensure that things are documented.

vmx · 2021-07-06T12:41:02Z

I opened an issue: #41 Everything else should be addressed (which unit offset is will be addressed when adding documentation).

vmx requested a review from dignifiedquire May 18, 2021 14:02

dignifiedquire reviewed May 20, 2021

View reviewed changes

src/opencl/error.rs Show resolved Hide resolved

dignifiedquire reviewed May 20, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

dignifiedquire reviewed May 20, 2021

View reviewed changes

src/opencl/mod.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed May 20, 2021

View reviewed changes

src/opencl/mod.rs Show resolved Hide resolved

dignifiedquire reviewed May 20, 2021

View reviewed changes

src/opencl/mod.rs Outdated Show resolved Hide resolved

vmx marked this pull request as draft May 20, 2021 16:56

vmx force-pushed the opencl3 branch 2 times, most recently from 82b6b1d to 9e0746b Compare May 31, 2021 12:53

vmx marked this pull request as ready for review May 31, 2021 14:01