[Mesa-dev] [RFC] clover: Attempt to make clover fail when a kernel fails to build
Francisco Jerez
currojerez at riseup.net
Fri May 16 03:11:16 PDT 2014
Tom Stellard <tom at stellard.net> writes:
> On Thu, May 15, 2014 at 10:42:07PM +0200, Francisco Jerez wrote:
>> Tom Stellard <tom at stellard.net> writes:
>>
>> > On Thu, May 15, 2014 at 06:22:11PM +0200, Francisco Jerez wrote:
>> >> Bruno Jimenez <brunojimen at gmail.com> writes:
>> >>
>> >> > Hi,
>> >> >
>> >> > I'm trying to make clover fail in the case that a kernel fails to build.
>> >> > My first attempt has been to track a llvm compile failure up from
>> >> > 'radeon_llvm_compile' (at radeon/radeon_llvm_emit.c), through
>> >> > 'r600_llvm_compile' (at r600/r600_llvm.c) and through
>> >> > 'evergreen_launch_grid' (at r600/evergreen_compute.c), then detect the
>> >> > failure when launching a kernel and throw an error.
>> >> >
>> >> > The attached patch is based on this, and it kind of works, as for the
>> >> > first kernel that fails to compile it will return the corresponding
>> >> > error (CL_INVALID_PROGRAM_EXECUTABLE), but for the following kernels it
>> >> > crashes completely.
>> >> >
>> >> > I think that this is not how we should detect compile errors, nor where
>> >> > we should. Because, after reading the OpenCL 1.1 spec, I think that they
>> >> > should be handled at 'clBuildProgram'.
>> >> >
>> >> > The spec for OpenCL 1.1 says about 'clBuildProgram':
>> >> > 'builds (compiles & links) a program executable [...]'
>> >> > And there's an error for build failures:
>> >> > 'CL_BUILD_PROGRAM_FAILURE if there is a failure to build the program
>> >> > executable'
>> >> >
>> >> > Any thoughts about this?
>> >>
>> >> Yeah... I think you're right and this is a dead end, it's probably not
>> >> the way we should handle this situation. Compiler errors should be
>> >> returned from the compiler, not from launch_grid(). This is likely to
>> >> be an LLVM back-end bug. Clang shouldn't return successfully if the
>> >> program is malformed and the pipe driver won't be able to deal with the
>> >> generated code. If the fact that clover::compile_program_llvm() doesn't
>> >> detect the failure is caused by radeon's two-step compilation process, I
>> >> guess that this is another reason to refactor the radeon compiler code
>> >> so the whole compilation happens in one step from the LLVM back-end and
>> >> most of what radeon_llvm_compile() does now is handled from within clang
>> >> as it's called from clover::compile_program_llvm(), which would generate
>> >> machine code directly instead of LLVM IR.
>> >>
>> >
>> > I've mostly implemented moving the compilation phase into clover, but it is
>> > dependent on this patch: http://lists.freedesktop.org/archives/mesa-dev/2014-March/055424.html
>> > until function calls are implemented, which may be a while.
>> >
>>
>> I don't think that patch would be required in principle if the function
>> inlining is taken care of by the LLVM back-end instead as we discussed
>> earlier.
>>
>
> The reason I've avoided doing this in the backend is that this would
> require re-running all the optimizations which would really hurt compile
> time. However, I think I am OK with having non-optimized inline
> functions, since the regular inliner will inline small functions anyway.
> This will motivate me to complete function call support.
>
Until you support actual function calls, couldn't you implement the same
mechanism as a target-specific function pass run before the function
inliner? (e.g. adding it to the pass manager from
AMDGPUTargetMachine::addAnalysisPasses)
>> > Is there some other way to handle this? Maybe a flag on struct pipe_compute_state?
>> >
>>
>> I guess another possibility would be to have clang run the necessary
>> checks somehow so we can be sure that a successful compilation implies
>> that the generated code is well-formed and the second compilation is
>> going to complete successfully too.
>>
>
> This would work, but I think I prefer your original idea.
>
> -Tom
>
>> Thanks.
>>
>> > -Tom
>> >
>> >> Thanks.
>> >>
>> >> > Bruno
>> >> > From e92bc2804b60ebb69268b02df7f57301aea8b20b Mon Sep 17 00:00:00 2001
>> >> > From: =?UTF-8?q?Bruno=20Jim=C3=A9nez?= <brunojimen at gmail.com>
>> >> > Date: Wed, 14 May 2014 17:04:50 +0200
>> >> > Subject: [PATCH] exit if llvm_compile fails
>> >> >
>> >> > ---
>> >> > src/gallium/drivers/ilo/ilo_gpgpu.c | 2 +-
>> >> > src/gallium/drivers/nouveau/nvc0/nvc0_compute.c | 2 +-
>> >> > src/gallium/drivers/nouveau/nvc0/nvc0_context.h | 8 ++++----
>> >> > src/gallium/drivers/nouveau/nvc0/nve4_compute.c | 2 +-
>> >> > src/gallium/drivers/r600/evergreen_compute.c | 12 ++++++++----
>> >> > src/gallium/drivers/radeonsi/si_compute.c | 2 +-
>> >> > src/gallium/include/pipe/p_context.h | 6 +++---
>> >> > src/gallium/state_trackers/clover/core/kernel.cpp | 14 +++++++++-----
>> >> > 8 files changed, 28 insertions(+), 20 deletions(-)
>> >> >
>> >> > diff --git a/src/gallium/drivers/ilo/ilo_gpgpu.c b/src/gallium/drivers/ilo/ilo_gpgpu.c
>> >> > index b17a518..19990fc 100644
>> >> > --- a/src/gallium/drivers/ilo/ilo_gpgpu.c
>> >> > +++ b/src/gallium/drivers/ilo/ilo_gpgpu.c
>> >> > @@ -32,7 +32,7 @@
>> >> > * This is a placeholder. We will need something similar to ilo_3d_pipeline.
>> >> > */
>> >> >
>> >> > -static void
>> >> > +static unsigned
>> >> > ilo_launch_grid(struct pipe_context *pipe,
>> >> > const uint *block_layout, const uint *grid_layout,
>> >> > uint32_t pc, const void *input)
>> >> > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c b/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c
>> >> > index ad287a2..26ace0a 100644
>> >> > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c
>> >> > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_compute.c
>> >> > @@ -193,7 +193,7 @@ nvc0_compute_upload_input(struct nvc0_context *nvc0, const void *input)
>> >> > }
>> >> > }
>> >> >
>> >> > -void
>> >> > +unsigned
>> >> > nvc0_launch_grid(struct pipe_context *pipe,
>> >> > const uint *block_layout, const uint *grid_layout,
>> >> > uint32_t label,
>> >> > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_context.h b/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
>> >> > index 76416a0..b1bbc54 100644
>> >> > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
>> >> > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
>> >> > @@ -349,11 +349,11 @@ nvc0_video_buffer_create(struct pipe_context *pipe,
>> >> > void nvc0_push_vbo(struct nvc0_context *, const struct pipe_draw_info *);
>> >> >
>> >> > /* nve4_compute.c */
>> >> > -void nve4_launch_grid(struct pipe_context *,
>> >> > - const uint *, const uint *, uint32_t, const void *);
>> >> > +unsigned nve4_launch_grid(struct pipe_context *,
>> >> > + const uint *, const uint *, uint32_t, const void *);
>> >> >
>> >> > /* nvc0_compute.c */
>> >> > -void nvc0_launch_grid(struct pipe_context *,
>> >> > - const uint *, const uint *, uint32_t, const void *);
>> >> > +unsigned nvc0_launch_grid(struct pipe_context *,
>> >> > + const uint *, const uint *, uint32_t, const void *);
>> >> >
>> >> > #endif
>> >> > diff --git a/src/gallium/drivers/nouveau/nvc0/nve4_compute.c b/src/gallium/drivers/nouveau/nvc0/nve4_compute.c
>> >> > index f243316..593d455 100644
>> >> > --- a/src/gallium/drivers/nouveau/nvc0/nve4_compute.c
>> >> > +++ b/src/gallium/drivers/nouveau/nvc0/nve4_compute.c
>> >> > @@ -428,7 +428,7 @@ nve4_compute_alloc_launch_desc(struct nouveau_context *nv,
>> >> > return (struct nve4_cp_launch_desc *)ptr;
>> >> > }
>> >> >
>> >> > -void
>> >> > +unsigned
>> >> > nve4_launch_grid(struct pipe_context *pipe,
>> >> > const uint *block_layout, const uint *grid_layout,
>> >> > uint32_t label,
>> >> > diff --git a/src/gallium/drivers/r600/evergreen_compute.c b/src/gallium/drivers/r600/evergreen_compute.c
>> >> > index 701bb5c..305bd84 100644
>> >> > --- a/src/gallium/drivers/r600/evergreen_compute.c
>> >> > +++ b/src/gallium/drivers/r600/evergreen_compute.c
>> >> > @@ -538,7 +538,7 @@ void evergreen_emit_cs_shader(
>> >> > RADEON_PRIO_SHADER_DATA));
>> >> > }
>> >> >
>> >> > -static void evergreen_launch_grid(
>> >> > +static unsigned evergreen_launch_grid(
>> >> > struct pipe_context *ctx_,
>> >> > const uint *block_layout, const uint *grid_layout,
>> >> > uint32_t pc, const void *input)
>> >> > @@ -550,6 +550,8 @@ static void evergreen_launch_grid(
>> >> >
>> >> > COMPUTE_DBG(ctx->screen, "*** evergreen_launch_grid: pc = %u\n", pc);
>> >> >
>> >> > + shader->active_kernel = kernel;
>> >> > + ctx->cs_shader_state.kernel_index = pc;
>> >> > #ifdef HAVE_OPENCL
>> >> >
>> >> > if (!kernel->code_bo) {
>> >> > @@ -566,7 +568,9 @@ static void evergreen_launch_grid(
>> >> > ctx->screen->has_compressed_msaa_texturing);
>> >> > bc->type = TGSI_PROCESSOR_COMPUTE;
>> >> > bc->isa = ctx->isa;
>> >> > - r600_llvm_compile(mod, ctx->b.family, bc, &use_kill, dump);
>> >> > +
>> >> > + if (r600_llvm_compile(mod, ctx->b.family, bc, &use_kill, dump))
>> >> > + return 1;
>> >> >
>> >> > if (dump && !sb_disasm) {
>> >> > r600_bytecode_disasm(bc);
>> >> > @@ -582,10 +586,10 @@ static void evergreen_launch_grid(
>> >> > ctx->b.ws->buffer_unmap(kernel->code_bo->cs_buf);
>> >> > }
>> >> > #endif
>> >> > - shader->active_kernel = kernel;
>> >> > - ctx->cs_shader_state.kernel_index = pc;
>> >> > evergreen_compute_upload_input(ctx_, block_layout, grid_layout, input);
>> >> > compute_emit_cs(ctx, block_layout, grid_layout);
>> >> > +
>> >> > + return 0;
>> >> > }
>> >> >
>> >> > static void evergreen_set_compute_resources(struct pipe_context * ctx_,
>> >> > diff --git a/src/gallium/drivers/radeonsi/si_compute.c b/src/gallium/drivers/radeonsi/si_compute.c
>> >> > index c0637f6..4ff7f7a 100644
>> >> > --- a/src/gallium/drivers/radeonsi/si_compute.c
>> >> > +++ b/src/gallium/drivers/radeonsi/si_compute.c
>> >> > @@ -117,7 +117,7 @@ static void si_set_global_binding(
>> >> > }
>> >> > }
>> >> >
>> >> > -static void si_launch_grid(
>> >> > +static unsigned si_launch_grid(
>> >> > struct pipe_context *ctx,
>> >> > const uint *block_layout, const uint *grid_layout,
>> >> > uint32_t pc, const void *input)
>> >> > diff --git a/src/gallium/include/pipe/p_context.h b/src/gallium/include/pipe/p_context.h
>> >> > index bc43530..0fa9278 100644
>> >> > --- a/src/gallium/include/pipe/p_context.h
>> >> > +++ b/src/gallium/include/pipe/p_context.h
>> >> > @@ -520,9 +520,9 @@ struct pipe_context {
>> >> > * should point to a buffer of at least
>> >> > * pipe_compute_state::req_input_mem bytes.
>> >> > */
>> >> > - void (*launch_grid)(struct pipe_context *context,
>> >> > - const uint *block_layout, const uint *grid_layout,
>> >> > - uint32_t pc, const void *input);
>> >> > + unsigned (*launch_grid)(struct pipe_context *context,
>> >> > + const uint *block_layout, const uint *grid_layout,
>> >> > + uint32_t pc, const void *input);
>> >> > /*@}*/
>> >> >
>> >> > /**
>> >> > diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp b/src/gallium/state_trackers/clover/core/kernel.cpp
>> >> > index 5e5fe51..5517b17 100644
>> >> > --- a/src/gallium/state_trackers/clover/core/kernel.cpp
>> >> > +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
>> >> > @@ -70,6 +70,7 @@ kernel::launch(command_queue &q,
>> >> > const auto reduced_grid_size =
>> >> > map(divides(), grid_size, block_size);
>> >> > void *st = exec.bind(&q);
>> >> > + unsigned err;
>> >> >
>> >> > // The handles are created during exec_context::bind(), so we need make
>> >> > // sure to call exec_context::bind() before retrieving them.
>> >> > @@ -89,11 +90,11 @@ kernel::launch(command_queue &q,
>> >> > q.pipe->set_global_binding(q.pipe, 0, exec.g_buffers.size(),
>> >> > exec.g_buffers.data(), g_handles.data());
>> >> >
>> >> > - q.pipe->launch_grid(q.pipe,
>> >> > - pad_vector(q, block_size, 1).data(),
>> >> > - pad_vector(q, reduced_grid_size, 1).data(),
>> >> > - find(name_equals(_name), m.syms).offset,
>> >> > - exec.input.data());
>> >> > + err = q.pipe->launch_grid(q.pipe,
>> >> > + pad_vector(q, block_size, 1).data(),
>> >> > + pad_vector(q, reduced_grid_size, 1).data(),
>> >> > + find(name_equals(_name), m.syms).offset,
>> >> > + exec.input.data());
>> >> >
>> >> > q.pipe->set_global_binding(q.pipe, 0, exec.g_buffers.size(), NULL, NULL);
>> >> > q.pipe->set_compute_resources(q.pipe, 0, exec.resources.size(), NULL);
>> >> > @@ -102,6 +103,9 @@ kernel::launch(command_queue &q,
>> >> > q.pipe->bind_sampler_states(q.pipe, PIPE_SHADER_COMPUTE, 0,
>> >> > exec.samplers.size(), NULL);
>> >> > exec.unbind();
>> >> > +
>> >> > + if (err)
>> >> > + throw error(CL_INVALID_PROGRAM_EXECUTABLE);
>> >> > }
>> >> >
>> >> > size_t
>> >> > --
>> >> > 1.9.2
>> >
>> >
>> >
>> >
>> >> _______________________________________________
>> >> 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/20140516/6ddb32f4/attachment-0001.sig>
More information about the mesa-dev
mailing list