[Mesa-dev] [PATCH v3 06/21] clover/api: Rework the validation of devices for building
Pierre Moreau
pierre.morrow at free.fr
Fri Feb 23 10:31:10 UTC 2018
On 2018-02-22 — 10:41, Francisco Jerez wrote:
> 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.
So, if I drop it, I should have something like this?
if (any_of([&](const device &dev) {
return !count(dev, valid_devs);
}, objs<allow_empty_tag>(d_devs, num_devs)))
auto devs = (d_devs ? objs(d_devs, num_devs) : valid_devs);
> > + 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())'.
No, because this is used by clLinkProgram as well, for which the valid devices
do not come from `prog.devices()`, but from `ctx.devices()` which might be
different.
> > + }, 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,
It was more like, since I am already validating the device list in
validate_build_common(), why not return the validated list as well, instead of
building it from scratch again, and potentially messing it up.
I need to have another look at that code, because there seems to be some
overlap with validate_link_devices.
> 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.
Ah, is this already taken care of by `objs(d_progs, num_progs)`? Good to know.
> > 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: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180223/4e169f74/attachment.sig>
More information about the mesa-dev
mailing list