[Mesa-dev] [PATCH 3/3] clover: add clLinkProgramm (CL 1.2)

[Francisco Jerez]

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

> ---
>  src/gallium/state_trackers/clover/api/program.cpp  | 35 ++++++++++++++++++++++
>  src/gallium/state_trackers/clover/core/error.hpp   |  7 +++++
>  src/gallium/state_trackers/clover/core/program.cpp |  4 +++
>  src/gallium/state_trackers/clover/core/program.hpp |  1 +
>  .../state_trackers/clover/llvm/invocation.cpp      |  2 +-
>  5 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/state_trackers/clover/api/program.cpp b/src/gallium/state_trackers/clover/api/program.cpp
> index 553bc83..7573933 100644
> --- a/src/gallium/state_trackers/clover/api/program.cpp
> +++ b/src/gallium/state_trackers/clover/api/program.cpp
> @@ -238,6 +238,41 @@ clCompileProgram(cl_program d_prog, cl_uint num_devs,
>     return e.get();
>  }
>  
> +CLOVER_API cl_program
> +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 {

You can drop the space between the function name an parenthesis.

> +   auto &ctx = obj(d_ctx);
> +   auto devs = (d_devs ? objs(d_devs, num_devs) :
> +                ref_vector<device>(ctx.devices()));
> +   auto opts = (p_opts ? p_opts : "");
> +   auto progs = objs(d_progs, num_progs);
> +
> +   if ((!pfn_notify && user_data))

One set of parentheses is redundant.

> +         throw error(CL_INVALID_VALUE);
> +
> +   if (any_of([&](const device &dev) {
> +            return !count(dev, ctx.devices());
> +         }, objs<allow_empty_tag>(d_devs, num_devs)))
> +      throw error(CL_INVALID_DEVICE);
> +
> +   auto prog = create<program>(ctx);

You could just pass empty arguments ({}) to the normal constructor of
program from binary to avoid defining a new one.

> +   try {
> +      prog().link(devs, opts, progs);
> +      *r_errcode = CL_SUCCESS;

The spec is not explicit about it, but I suspect that the r_errcode
argument was intended to be optional, you should probably use
ret_error() here and in the cases below to avoid a crash.

> +   } catch (link_options_error &e) {
> +      throw;
> +   } catch (error &e) {
> +      *r_errcode = CL_LINK_PROGRAM_FAILURE;
> +   }

If you throw link_error consistently from program::link() you should be
able to care about one exception type here only, like:

|   } catch (link_error &e) {
|      ret_error(r_errcode, e);
|   }

> +
> +   return ret_object(prog);
> +} catch (error &e) {
> +   ret_error(r_errcode, e);
> +   return NULL;
> +}
> +
>  CLOVER_API cl_int
>  clUnloadCompiler() {
>     return CL_SUCCESS;
> diff --git a/src/gallium/state_trackers/clover/core/error.hpp b/src/gallium/state_trackers/clover/core/error.hpp
> index 4ec619c..f6c55a3 100644
> --- a/src/gallium/state_trackers/clover/core/error.hpp
> +++ b/src/gallium/state_trackers/clover/core/error.hpp
> @@ -79,6 +79,13 @@ namespace clover {
>        }
>     };
>  
> +   class link_options_error : public error {
> +   public:
> +      link_options_error(const std::string &what = "") :
> +         error(CL_INVALID_LINKER_OPTIONS , what) {
> +      }
> +   };
> +

Doesn't this (and the hunk below where you change link_program_llvm() to
use it) belong in a different patch?  E.g. wherever link_program_llvm()
is implemented?

>     template<typename O>
>     class invalid_object_error;
>  
> diff --git a/src/gallium/state_trackers/clover/core/program.cpp b/src/gallium/state_trackers/clover/core/program.cpp
> index 4aa2622..61fb603 100644
> --- a/src/gallium/state_trackers/clover/core/program.cpp
> +++ b/src/gallium/state_trackers/clover/core/program.cpp
> @@ -24,6 +24,10 @@
>  
>  using namespace clover;
>  
> +program::program(clover::context &ctx) :
> +   has_source(false), context(ctx), _kernel_ref_counter(0) {
> +}
> +
>  program::program(clover::context &ctx, const std::string &source) :
>     has_source(true), context(ctx), _source(source), _kernel_ref_counter(0) {
>  }
> diff --git a/src/gallium/state_trackers/clover/core/program.hpp b/src/gallium/state_trackers/clover/core/program.hpp
> index 7d86018..c24ad83 100644
> --- a/src/gallium/state_trackers/clover/core/program.hpp
> +++ b/src/gallium/state_trackers/clover/core/program.hpp
> @@ -37,6 +37,7 @@ namespace clover {
>           evals, const std::vector<intrusive_ref<device>> &> device_range;
>  
>     public:
> +      program(clover::context &ctx);
>        program(clover::context &ctx,
>                const std::string &source);
>        program(clover::context &ctx,
> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> index d115f15..2bf7775 100644
> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> @@ -782,7 +782,7 @@ clover::link_program_llvm(const std::vector<module> &modules,
>     clang::CompilerInstance c;
>     if (!create_from_arg_llvm(c, target, options, s_log)) {
>        r_log = log;
> -      throw error(CL_INVALID_LINKER_OPTIONS);
> +      throw link_options_error();
>     }
>  
>     llvm::Module linked_mod("link", llvm_ctx);
> -- 
> 2.5.0.rc2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150725/2a23e10d/attachment.sig>