[Mesa-dev] i965 implementation of the ARB_shader_image_load_store built-ins. (v2)

Tue May 5 14:17:14 PDT 2015

Kenneth Graunke <kenneth at whitecape.org> writes:

> On Saturday, May 02, 2015 06:29:27 PM Francisco Jerez wrote:
>> This v2 is motivated by the less than enthusiastic reception of my
>> last two series implementing the image load, store and atomic
>> intrinsics in the i965 back-end.  It substitutes both.
>> 
>> The built-ins are now implemented in the scalar back-end only, what
>> means that IVB and HSW are no longer able to expose image support for
>> the VS and GS stages -- behaviour acceptable according to the
>> specification.  BDW and up should still be able to expose images in
>> the VS stage, and possibly in the GS stage at some point too.  CS
>> images can be supported on all gens (that support CS that is).  The
>> benefit is that the FS/VEC4 abstraction becomes unnecessary and the
>> overall set of changes amounts to less code.
>> 
>> It can be found in the following branch along with its dependencies:
>> http://cgit.freedesktop.org/~currojerez/mesa/log/?h=image-load-store-scalar
>> 
>> src/mesa/drivers/dri/i965/Makefile.sources           |    4 +
>> src/mesa/drivers/dri/i965/brw_context.c              |   12 ++
>> src/mesa/drivers/dri/i965/brw_fs.cpp                 |   43 ++++-
>> src/mesa/drivers/dri/i965/brw_fs.h                   |   19 +-
>> src/mesa/drivers/dri/i965/brw_fs_builder.h           |  617 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> src/mesa/drivers/dri/i965/brw_fs_generator.cpp       |    8 +-
>> src/mesa/drivers/dri/i965/brw_fs_nir.cpp             |   56 ++++--
>> src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp | 1144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> src/mesa/drivers/dri/i965/brw_fs_surface_builder.h   |  233 ++++++++++++++++++++++++
>> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp         |  466 ++++++++++++++++++++++++++++-------------------
>> src/mesa/drivers/dri/i965/brw_ir_fs.h                |   56 ++++++
>> src/mesa/drivers/dri/i965/brw_ir_vec4.h              |   55 ++++++
>> src/mesa/drivers/dri/i965/brw_shader.cpp             |   32 ++++
>> src/mesa/drivers/dri/i965/brw_shader.h               |    6 +
>> src/mesa/drivers/dri/i965/brw_vec4.cpp               |   10 +
>> src/mesa/drivers/dri/i965/brw_vec4.h                 |    6 +
>> src/mesa/drivers/dri/i965/brw_vec4_builder.h         |  579 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> src/mesa/drivers/dri/i965/brw_vec4_generator.cpp     |    8 +-
>> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp       |  179 ++++++++++--------
>> src/mesa/drivers/dri/i965/intel_extensions.c         |    2 +
>> 20 files changed, 3224 insertions(+), 311 deletions(-)
>> 
>> [PATCH 01/29] i965/fs: Have component() set the register stride to zero.
>> [PATCH 02/29] i965: Add register constructors taking an backend_reg as argument.
>> [PATCH 03/29] i965: Define consistent interface to disable control flow execution masking.
>> [PATCH 04/29] i965: Define consistent interface to predicate an instruction.
>> [PATCH 05/29] i965: Define consistent interface to enable instruction conditional modifiers.
>> [PATCH 06/29] i965: Define consistent interface to enable instruction result saturation.
>> [PATCH 07/29] i965/fs: Introduce FS IR builder.
>> [PATCH 08/29] i965/vec4: Introduce VEC4 IR builder.
>> [PATCH 09/29] i965: Define the setup_vector_uniform_values() backend_visitor interface.
>> [PATCH 10/29] i965: Add support for handling image uniforms to the GLSL IR visitors.
>> [PATCH 11/29] i965: Add a visitor method to extract the result of a visit.
>> [PATCH 12/29] i965/fs: Obtain atomic counter locations by recursing through the visitor.
>> [PATCH 13/29] i965/vec4: Obtain atomic counter locations by recursing through the visitor.
>> [PATCH 14/29] i965: Lift the constness restriction on surface indices passed to untyped ops.
>> [PATCH 15/29] i965/fs: Import array utils for the surface message builder.
>> [PATCH 16/29] i965/fs: Import helpers to convert vectors into arrays and back.
>> [PATCH 17/29] i965/fs: Import surface message builder functions.
>> [PATCH 18/29] i965/fs: Import image access validity checks.
>> [PATCH 19/29] i965/fs: Import image memory offset calculation code.
>> [PATCH 20/29] i965/fs: Import image format metadata queries.
>> [PATCH 21/29] i965/fs: Import image format conversion primitives.
>> [PATCH 22/29] i965/fs: Implement image load, store and atomic.
>> [PATCH 23/29] i965/fs: Revisit GLSL IR atomic counter intrinsic translation.
>> [PATCH 24/29] i965/fs: Revisit NIR atomic counter intrinsic translation.
>> [PATCH 25/29] i965/fs: Import GLSL IR image intrinsic translation code.
>> [PATCH 26/29] i965/fs: Import GLSL IR memory barrier intrinsic translation code.
>> [PATCH 27/29] i965/fs: Drop unused untyped surface read and atomic emit methods.
>> [PATCH 28/29] i965: Define implementation constants for ARB_shader_image_load_store.
>> [PATCH 29/29] i965: Expose ARB_shader_image_load_store.
>
> Hi Curro,
>
Hi Ken,

> Thanks for sending this out.  I'm definitely more comfortable with v2,
> though I do still have concerns.
>
> At this point, NIR is the default FS backend.  I believe our plan is to
> keep the GLSL IR backend for the Mesa 10.6 release, so that people can
> try INTEL_USE_NIR=0 if they encounter any regressions.  But, after 10.6
> branches (later this month!), we're free to delete it entirely.
>
> So, I don't think it makes sense to add new features to the deprecated
> non-NIR scalar shader backend.  It's going away soon.  (Igalia is also
> working on vec4 NIR support - so we hope to switch that over eventually
> too.  It'll probably take a while, though.)  We should only do NIR.
>
Right, I'll send a v3 shortly adding NIR support and dropping the GLSL
IR implementation.  Some of the clean-up patches that affect the GLSL IR
visitors seem nice to have though, even if they are going to be removed
at some point in the future.

> That then begs the question - could we do the format conversion and
> address calculations in a i965-specific NIR pass?  It could simplify
> the backend changes.  NIR also has better CSE, which could help for
> repeated image access.  

I doubt that rewriting things in NIR would be of any help at this point.
We could have support for ARB_shader_image_load_store already in the
10.6 release if we refrain from reimplementing the world in the last
minute.  And I doubt that an implementation in terms of NIR would have
simplified anything, it would have required a bunch of intrinsics with
driver-specific semantics, and I must admit that I find generating NIR
even more annoying than generating i965 IR directly (for an example of
what a mean see how [1] spends over twenty lines to calculate a simple
"a + b * c" expression).

>
> We may even be able to regain vec4 support once Igalia's vec4-NIR
> backend matures.

I guess we could regain vec4 support either way if we don't mess up the
design of the next SSA-based back-end IR in a way that again prevents
any useful code sharing between the scalar and vec4 back-ends...

> I like the idea of the builder refactor - having a mechanism for emitting
> code at arbitrary points makes a ton of sense.  But I'm uncomfortable
> with how much code is added - and duplicated from the existing visitor
> code - given that fs_visitor is refactored to use it.
>
> I agree with Jason that we should discuss our goal - what things should
> look like - but frankly, I think we should hold off on large scale
> refactoring until after we delete the GLSL IR backend.  There will be
> much less code to reorganize.
>
I agree, I wasn't planning on migrating the rest of the compiler
back-end to the builder framework until that happens.

> It may also make sense to land Skylake support first - I believe we can
> use typed surface messages in most places, which is a lot easier.  We
> could land that, then add the additional complexity to support BDW/HSW.
>
> --Ken

[1] http://cgit.freedesktop.org/mesa/mesa/tree/src/glsl/nir/nir_lower_atomics.c#n84
-------------- 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/20150506/4c7f203b/attachment-0001.sig>