[Mesa-dev] [PATCH 0/3] cl workdim v2

[Francisco Jerez]

Jan Vesely <jan.vesely at rutgers.edu> writes:

> On Thu, 2014-08-07 at 16:02 +0300, Francisco Jerez wrote:
>> Jan Vesely <jan.vesely at rutgers.edu> writes:
>> 
>> > This respin includes Francisco's approach of providing implicit
>> > in the arg vector passed from clover, and Tom's idea of appending
>> > implicit args after the kernel args.
>> >
>> 
>> Hmmm...  Maybe it would make sense to add some sort of versioning
>> (e.g. as part of the target triple) to the binary interface between
>> clover and the kernel instead, so we can handle this sort of
>> non-backwards compatible changes and the compiler back-end and libclc
>> have some way to find out whether some specific feature is available and
>> e.g. some specific extension should be enabled.
>> 
>> > I assumed it's not safe to modify exec.input, so the input vector is copied
>> > before appending work dim.
>> >
>> 
>> Why wouldn't it be safe?  You just need to make sure they're appended
>> before the compute state is created.
>
> I thought there might be a problem when called from multiple threads,
> but it looks like most of the vars are local to the current call anyway.
>
> I looked at the code a bit better, and need a bit of help with what the
> proffered approach would be.
>
> exec_context::bind() appends all kernel args to the input vector. If the
> implicit args are added before bind() it shifts all other args, which is
> not what we want.
> if the implicit args are appended after, they are not accounted for in
> shader->input_size (and not copied by the driver).
>
> my current code modifies exec_context::bind() to preserve the content of
> input before binding kernel args, and append the old content after the
> args are bound.
> I have also considered passing and implicit args vector to
> exec_context::bind to make the trick more visible.
>
> Turning workdim into a proper arg in _args does not work either, because
> it is not present in module args.
>
> any thoughts?
>

I finally had a chance to take a closer look at your series.  It looks
like you're right: In order to implement my proposal cleanly, implicit
arguments would have to be part of the _args array so the compiler would
have to include them in the module argument lists with memory layout
parameters (e.g. alignment, size) suitable for the hardware, so there's
probably little benefit compared to your original approach that includes
the number of dimensions as an additional launch_grid() parameter.

So we don't have to change it again, can you add another array parameter
for the base grid offset?  That's another thing we don't pass through
the pipe driver API currently and CL requires. The prototype of
launch_grid could look like:

| void (*launch_grid)(struct pipe_context *context, uint dims,
|                     const uint *block_layout, const uint *grid_layout,
|                     const uint *grid_offset, uint32_t pc,
|                     const void *input);

And don't forget to update the docs. :)

Thank you.

> thanks,
> jan
>
>
>> 
>> > Passes get-work-dim piglit on turks without any regression,
>> > I have not tested SI as I don't have the hw.
>> >
>> > jan
>> >
>> >
>> >
>> >
>> > Jan Vesely (3):
>> >   gallium: Pass input data size to launch_grid
>> >   clover: Add work dimension implicit param to input
>> >   r600,radeonsi: Copy implicit args provided by clover
>> >
>> >  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   |   4 +-
>> >  src/gallium/drivers/nouveau/nvc0/nve4_compute.c   |   2 +-
>> >  src/gallium/drivers/r600/evergreen_compute.c      |  14 +-
>> >  src/gallium/drivers/r600/evergreen_compute.h      |   1 -
>> >  src/gallium/drivers/radeonsi/si_compute.c         |   6 +-
>> >  src/gallium/include/pipe/p_context.h              |   2 +-
>> >  src/gallium/state_trackers/clover/core/kernel.cpp | 162 ++++++++++++----------
>> >  src/gallium/tests/trivial/compute.c               |  40 +++---
>> >  10 files changed, 122 insertions(+), 113 deletions(-)
>> >
>> > -- 
>> > 1.9.3
>
> -- 
> Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140816/14b9c59d/attachment.sig>