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

Mon May 4 16:17:13 PDT 2015

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,

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.

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.  We may even be able to regain vec4 support
once Igalia's vec4-NIR backend matures.

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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150504/954e93a9/attachment.sig>