[Parallel_libs-dev] StreamExecutor design questions

[James Price]

Hi Jason,

> 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.

That’s halfway to what I was thinking. If PlatformDevice is the thing that creates the objects, it makes sense for it to also be in charge of destroying them with the destroyKernel() and destroyStream() methods you propose, and this is indeed more consistent with the device memory handling.

However, this makes it even more unclear (to me) what the benefit is of having the Platform{Kernel,Stream}Handle wrapper classes at all. Why not just have createKernel() and createStream() return a raw void* which is then stored directly in the Kernel and Stream classes? The raw pointers could then be passed to PlatformDevice as needed, including to the new destroy*() methods, and the platform implementor no longer has to create a couple of subclasses to hold those raw pointers with an extra level of indirection.

If, however, the Platform*Handle classes are intended to also serve another purpose in the future, then it probably makes sense to keep them. I’m no C++ design guru, so I’m also happy to accept if the wrapper class approach is simply better practice or safer in some way than raw pointers.


> 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?

A ‘shutdown everything in the platform’ function that is called automatically as Justin proposed feels like it would be a good solution.


James

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/parallel_libs-dev/attachments/20160902/828e94d6/attachment.html>