[Mesa-dev] [PATCH v3 06/21] clover/api: Rework the validation of devices for building

[Francisco Jerez]

Pierre Moreau <pierre.morrow at free.fr> writes:

> Signed-off-by: Pierre Moreau <pierre.morrow at free.fr>
> ---
>  src/gallium/state_trackers/clover/api/program.cpp  | 39 +++++++++++++---------
>  src/gallium/state_trackers/clover/core/program.cpp |  3 +-
>  2 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/api/program.cpp b/src/gallium/state_trackers/clover/api/program.cpp
> index 9d59668f8f..babe45ccde 100644
> --- a/src/gallium/state_trackers/clover/api/program.cpp
> +++ b/src/gallium/state_trackers/clover/api/program.cpp
> @@ -29,9 +29,10 @@
>  using namespace clover;
>  
>  namespace {
> -   void
> +   ref_vector<device>
>     validate_build_common(const program &prog, cl_uint num_devs,
>                           const cl_device_id *d_devs,
> +                         ref_vector<device> &valid_devs,
>                           void (*pfn_notify)(cl_program, void *),
>                           void *user_data) {
>        if (!pfn_notify && user_data)
> @@ -40,10 +41,16 @@ namespace {
>        if (prog.kernel_ref_count())
>           throw error(CL_INVALID_OPERATION);
>  
> +      if ((!d_devs && num_devs > 0u) || (d_devs && num_devs == 0u))
> +         throw error(CL_INVALID_VALUE);
> +

This check shouldn't be necessary, it was provided by the call to the
objs<allow_empty_tag>() CL argument processing helper you removed below.

> +      auto devs = (d_devs ? objs(d_devs, num_devs) : valid_devs);
>        if (any_of([&](const device &dev) {
> -               return !count(dev, prog.context().devices());
> -            }, objs<allow_empty_tag>(d_devs, num_devs)))
> +               return !count(dev, valid_devs);

This should probably be '!count(dev, prog.devices())'.

> +            }, devs))
>           throw error(CL_INVALID_DEVICE);
> +
> +      return devs;

The benefit from calculating the device list in validate_build_common()
seems a bit dubious to me, but if you want to share the one ternary
operator I'd split the current validate_build_common() into two
functions: 'void validate_build_common(prog, pfn_notify, user_data)'
that only validates the program object and pfn_notify closure and
'ref_vector<device> validate_build_devices(prog, num_devs, d_devs)' that
does the device checks and returns the correct device set (Note that
there is no need for the caller to provide the set of valid devices as
argument as you're doing here, it should always be equal to
prog.devices()).  Then I'd replace the all_devs argument of
validate_link_devices() with a num_devs/d_devs pair and call
validate_build_devices() from there.

>     }
>  }
>  
> @@ -176,13 +183,12 @@ clBuildProgram(cl_program d_prog, cl_uint num_devs,
>                 void (*pfn_notify)(cl_program, void *),
>                 void *user_data) try {
>     auto &prog = obj(d_prog);
> -   auto devs = (d_devs ? objs(d_devs, num_devs) :
> -                ref_vector<device>(prog.context().devices()));
> +   auto valid_devs = ref_vector<device>(prog.devices());
> +   auto devs = validate_build_common(prog, num_devs, d_devs, valid_devs,
> +                                     pfn_notify, user_data);
>     const auto opts = std::string(p_opts ? p_opts : "") + " " +
>                       debug_get_option("CLOVER_EXTRA_BUILD_OPTIONS", "");
>  
> -   validate_build_common(prog, num_devs, d_devs, pfn_notify, user_data);
> -
>     if (prog.has_source) {
>        prog.compile(devs, opts);
>        prog.link(devs, opts, { prog });
> @@ -202,14 +208,13 @@ clCompileProgram(cl_program d_prog, cl_uint num_devs,
>                   void (*pfn_notify)(cl_program, void *),
>                   void *user_data) try {
>     auto &prog = obj(d_prog);
> -   auto devs = (d_devs ? objs(d_devs, num_devs) :
> -                ref_vector<device>(prog.context().devices()));
> +   auto valid_devs = ref_vector<device>(prog.devices());
> +   auto devs = validate_build_common(prog, num_devs, d_devs, valid_devs,
> +                                     pfn_notify, user_data);
>     const auto opts = std::string(p_opts ? p_opts : "") + " " +
>                       debug_get_option("CLOVER_EXTRA_COMPILE_OPTIONS", "");
>     header_map headers;
>  
> -   validate_build_common(prog, num_devs, d_devs, pfn_notify, user_data);
> -
>     if (bool(num_headers) != bool(header_names))
>        throw error(CL_INVALID_VALUE);
>  
> @@ -275,16 +280,18 @@ clLinkProgram(cl_context d_ctx, cl_uint num_devs, const cl_device_id *d_devs,
>                const char *p_opts, cl_uint num_progs, const cl_program *d_progs,
>                void (*pfn_notify) (cl_program, void *), void *user_data,
>                cl_int *r_errcode) try {
> +   if (num_progs == 0u || (num_progs != 0u && !d_progs))
> +      throw error(CL_INVALID_VALUE);
> +

This check is already taken care of by the common CL argument
validation, please drop it.

>     auto &ctx = obj(d_ctx);
>     const auto opts = std::string(p_opts ? p_opts : "") + " " +
>                       debug_get_option("CLOVER_EXTRA_LINK_OPTIONS", "");
>     auto progs = objs(d_progs, num_progs);
>     auto prog = create<program>(ctx);
> -   auto devs = validate_link_devices(progs,
> -                                     (d_devs ? objs(d_devs, num_devs) :
> -                                      ref_vector<device>(ctx.devices())));
> -
> -   validate_build_common(prog, num_devs, d_devs, pfn_notify, user_data);
> +   auto valid_devs = ref_vector<device>(ctx.devices());
> +   auto devs = validate_build_common(prog, num_devs, d_devs, valid_devs,
> +                                     pfn_notify, user_data);
> +   devs = validate_link_devices(progs, devs);
>  
>     try {
>        prog().link(devs, opts, progs);
> diff --git a/src/gallium/state_trackers/clover/core/program.cpp b/src/gallium/state_trackers/clover/core/program.cpp
> index ae4b50a879..1a4a75b961 100644
> --- a/src/gallium/state_trackers/clover/core/program.cpp
> +++ b/src/gallium/state_trackers/clover/core/program.cpp
> @@ -27,7 +27,8 @@
>  using namespace clover;
>  
>  program::program(clover::context &ctx, const std::string &source) :
> -   has_source(true), context(ctx), _source(source), _kernel_ref_counter(0) {
> +   has_source(true), context(ctx), _devices(ctx.devices()), _source(source),
> +   _kernel_ref_counter(0) {
>  }
>  
>  program::program(clover::context &ctx,
> -- 
> 2.16.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180222/0a77ab4f/attachment.sig>