<div dir="ltr">Hi James,<div><br></div><div>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.</div><div><br></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif">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.</span><br></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif"><br></span></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif">My idea with </span><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif;line-height:1.5">PlatformKernelHandle and </span><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif;line-height:1.5">PlatformStreamHandle was that their destructors would free any resources they held. So, for example, in the case of your </span>OCLStreamHandle class, I was thinking that you would also define a destructor that called clReleaseCommandQueue.</div><div><br></div><div>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. <span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif;line-height:1.5">PlatformKernelHandle and </span><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif;line-height:1.5">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 </span><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif">PlatformKernelHandle and PlatformStreamHandle would call the corresponding Device destroy functions just like GlobalDeviceMemoryBase calls the freeDeviceMemory function in its destructor.</font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><br></font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif">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.</font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><br></font></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif">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.</span><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><br></font></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif"><br></span></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif">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?</font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><br></font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif">- If you *do* want the PlatformKernelHandle class, PlatformInterfaces.cpp was missing its destructor implementation.<br></font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><br></font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif">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.</font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><br></font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif">- 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.<br></font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><br></font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif">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.</font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><br></font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><div>- Missing LICENSE.txt at the top-level parallel-libs directory?</div><div><br></div><div>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.</div><div><br></div><div>Thanks for all your help with this project,</div><div><br></div><div>-Jason</div></font></div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 2, 2016 at 12:32 PM James Price <<a href="mailto:J.Price@bristol.ac.uk">J.Price@bristol.ac.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
I’ve thrown together a quick and dirty OpenCL backend for SE to play around with writing some application code that uses it, and as a result have one or two design questions. I know it’s early days so I apologise if I’m jumping the gun.<br>
<br>
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.<br>
<br>
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.<br>
<br>
Other minor points that I came across:<br>
<br>
- If you *do* want the PlatformKernelHandle class, PlatformInterfaces.cpp was missing its destructor implementation.<br>
<br>
- 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.<br>
<br>
- Missing LICENSE.txt at the top-level parallel-libs directory?<br>
<br>
For reference, I’ve thrown this OpenCL backend up here (it’s not great, but it works):<br>
<a href="https://gist.github.com/jrprice/84e3f80cf0c851041d274a663d7ef73f" rel="noreferrer" target="_blank">https://gist.github.com/jrprice/84e3f80cf0c851041d274a663d7ef73f</a><br>
<br>
Cheers,<br>
<br>
James<br>
<br>
</blockquote></div>