[Mesa-dev] clover : clEnqueueMarkerWithWaitList and clEnqueueBarrierWithWaitList

[Francisco Jerez]

EdB <edb+mesa at sigluy.net> writes:

> Hello
>
> I would be interested in developing for clover.
> As a first exercise I implement clEnqueueMarkerWithWaitList and clEnqueueBarrierWithWaitList needed for OpenCL 1.2
>
> This patch is more a proof of concept than a real one.
>
> Please tell me if I get it wrong.

Thanks for looking into this, some suggestions below.

> diff --git a/src/gallium/state_trackers/clover/api/dispatch.cpp b/src/gallium/state_trackers/clover/api/dispatch.cpp
> index 746372c..e4f7ea3 100644
> --- a/src/gallium/state_trackers/clover/api/dispatch.cpp
> +++ b/src/gallium/state_trackers/clover/api/dispatch.cpp
> @@ -129,8 +129,8 @@ namespace clover {
>        NULL, // clEnqueueFillBuffer
>        NULL, // clEnqueueFillImage
>        NULL, // clEnqueueMigrateMemObjects
> -      NULL, // clEnqueueMarkerWithWaitList
> -      NULL, // clEnqueueBarrierWithWaitList
> +      clEnqueueMarkerWithWaitList,
> +      clEnqueueBarrierWithWaitList,
>        NULL, // clGetExtensionFunctionAddressForPlatform
>        NULL, // clCreateFromGLTexture
>        NULL, // clGetDeviceIDsFromD3D11KHR
> diff --git a/src/gallium/state_trackers/clover/api/event.cpp b/src/gallium/state_trackers/clover/api/event.cpp
> index 6b1956c..e0349a7 100644
> --- a/src/gallium/state_trackers/clover/api/event.cpp
> +++ b/src/gallium/state_trackers/clover/api/event.cpp
> @@ -179,6 +179,37 @@ clEnqueueMarker(cl_command_queue d_q, cl_event *rd_ev) try {
>  }
>  
>  CLOVER_API cl_int
> +clEnqueueMarkerWithWaitList(cl_command_queue d_q, cl_uint num_evs,
> +                            const cl_event *d_evs, cl_event *rd_ev) try {
> +   if (!d_evs and num_evs > 0)
> +      throw error(CL_INVALID_EVENT_WAIT_LIST);
> +
> +   if (d_evs and !num_evs)
> +      throw error(CL_INVALID_EVENT_WAIT_LIST);
> +
> +   auto &q = obj(d_q);
> +
> +   ref_vector<event> evs = {};
> +
> +   if (num_evs)
> +      evs = objs(d_evs, num_evs);
> +

The wait_list_tag specialization of objs() already takes care of the
argument validation with wait-list-like semantics.  You could replace
all the code above with something like:

|   auto &q = obj(d_q);
|   auto deps = objs<wait_list_tag>(d_deps, num_deps);

> +   for (auto &ev : evs) {
> +      if (ev.context() != q.context())
> +         throw error(CL_INVALID_CONTEXT);
> +   }
> +
> +   auto hev = new hard_event(q, CL_COMMAND_MARKER, evs);
> +   if (rd_ev)
> +      *rd_ev = desc(hev);
> +
> +   return CL_SUCCESS;
> +

This is going to leak memory whenever 'rd_ev' is NULL, how about:

|   // Create a hard event that depends on the events in the wait list.
|   auto hev = create<hard_event>(q, CL_COMMAND_MARKER, deps);
|
|   ret_object(rd_ev, hev);
|   return CL_SUCCESS;

The create() helper creates an instance of some object and returns it as
a smart pointer that takes ownership of the object, so it's less likely
to make mistakes like this.

> +} catch (error &e) {
> +   return e.get();
> +}
> +
> +CLOVER_API cl_int
>  clEnqueueBarrier(cl_command_queue d_q) try {
>     obj(d_q);
>  
> @@ -191,6 +222,40 @@ clEnqueueBarrier(cl_command_queue d_q) try {
>  }
>  
>  CLOVER_API cl_int
> +clEnqueueBarrierWithWaitList(cl_command_queue d_q, cl_uint num_evs,
> +                             const cl_event *d_evs, cl_event *rd_ev) try {
> +   if (!d_evs and num_evs > 0)
> +      throw error(CL_INVALID_EVENT_WAIT_LIST);
> +
> +   if (d_evs and !num_evs)
> +      throw error(CL_INVALID_EVENT_WAIT_LIST);
> +
> +   auto &q = obj(d_q);
> +
> +   ref_vector<event> evs = {};
> +
> +   if (num_evs)
> +      evs = objs(d_evs, num_evs);
> +

Same as before:
|   auto &q = obj(d_q);
|   auto deps = objs<wait_list_tag>(d_deps, num_deps);

> +   for (auto &ev : evs) {
> +      if (ev.context() != q.context())
> +         throw error(CL_INVALID_CONTEXT);
> +   }
> +
> +
> +#define TMP_CL_COMMAND_BARRIER 0x1205  //a temporary DEFINE, it needed to be in cl.h

We should probably update the CL headers to 1.2 before implementing
this.

> +   auto hev = new hard_event(q, TMP_CL_COMMAND_BARRIER, evs);
> +   if (rd_ev)
> +      *rd_ev = desc(hev);
> +#undef TMP_CL_COMMAND_BARRIER
> +

Use create<hard_event>() and ret_object() as before.

How about implementing clEnqueueWaitForEvents() in terms of this entry
point?  It should be equivalent to:

|   // The wait list is mandatory for clEnqueueWaitForEvents().
|   objs(d_evs, num_evs);
|
|   clEnqueueBarrierWithWaitList(d_q, num_evs, d_evs, NULL);

Thank you.

> +   return CL_SUCCESS;
> +
> +} catch (error &e) {
> +   return e.get();
> +}
> +
> +CLOVER_API cl_int
>  clEnqueueWaitForEvents(cl_command_queue d_q, cl_uint num_evs,
>                         const cl_event *d_evs) try {
>     auto &q = obj(d_q);
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140423/2bda5302/attachment.sig>