I’m glad to hear you are experimenting with OpenCL in SE, and I really appreciate your insights and questions about the design. I definitely don’t think it’s too early for questions like these, and I hope you will continue to ask any questions you have in the future as well.
What’s the motivation for having the PlatformKernelHandle and PlatformStreamHandle classes, rather than just using opaque pointers? These classes result in a some extra subclassing when implementing an SE backend, just to wrap a platform specific pointer (e.g. cl_kernel or cl_command_queue), but maybe I’m missing the point here. It also seems to be slightly inconsistent with the device memory stuff, which uses raw opaque pointers at the PlatformDevice level.
My idea with PlatformKernelHandle and PlatformStreamHandle was that their destructors would free any resources they held. So, for example, in the case of your OCLStreamHandle class, I was thinking that you would also define a destructor that called clReleaseCommandQueue.
I agree this is inconsistent with the way device memory is currently handled using a void* handle, and I’m glad you pointed that out. The device memory object currently handles its own cleanup by making a call to PlatformDevice::freeDeviceMemory, so we could create consistency by following this model. PlatformKernelHandle and PlatformStreamHandle would each store a void* opaque handle to the platform-specific implementation and new functions would be added to the PlatformDevice interface to destroyKernel(PlatformKernelHandle*) and destroyStream(PlatformStreamHandle*). Then the destructors for PlatformKernelHandle and PlatformStreamHandle would call the corresponding Device destroy functions just like GlobalDeviceMemoryBase calls the freeDeviceMemory function in its destructor.
Now that I’ve laid it out this way, I think you’re right that that design would be better. It would be consistent and it would be easier to implement a platform. Does that sound like the right design to you, as well? If so, I’ll write up some patches to make those changes.
It also seems that there is no way for a user to fully release the resources used by a SE device, since the end-user doesn’t own the Device pointer. Maybe I’m worrying about nothing here, but I wonder whether there may be some devices that become ‘unhappy’ if you don’t release their contexts/default queues etc before the application exits.
I think you are right to say there is no way for a user to free an SE device. I don’t know if anyone will ever need to free those resources, but I think we will want to add a method to the platform class to free the resources for a device with a given ordinal. Of course that might leave dangling references to a device that has been freed, so we might want to have the platform pass out std::shared_ptr to Device instead of raw Device*. In any case, I think we have a few good options here that won’t be hard to change later. What do you think?
- If you do want the PlatformKernelHandle class, PlatformInterfaces.cpp was missing its destructor implementation.
Good catch. As per the discussion above, I will probably be adding it in with a non-default body if we switch to using void* handles.
- Should Device have a getName() that forwards onto PlatformDevice? Otherwise there seems to be no way for the end-user to get at the name.
I think that’s a great idea. I can add a patch for it, or you can feel free to submit a patch as well and I’ll be glad to review it and check it in as needed.
- Missing LICENSE.txt at the top-level parallel-libs directory?
You’re probably right, but I want to check with chandlerc when he gets back from vacation next week. Since parallel-libs is supposed to be made up of different sub-projects, I think there’s some possibility that there might be different licenses. I’ll try to figure out if I should put a LICENSE.txt in the top-level, the streamexecutor subdirectory, or both.
Thanks for all your help with this project,