[Mesa-dev] [PATCH v5 00/70] ARB_shader_storage_buffer_object (mesa, i965)

[Kristian Høgsberg]

On Thu, Sep 10, 2015 at 03:35:16PM +0200, Iago Toral Quiroga wrote:
> Hi,
> 
> this is the latest version of the ARB_shader_storage_buffer_object
> implementation. A good part of the frontend bits for this are already in
> master, but this adds some more missing pieces, specifically std430 and
> memory qualifiers. Additionally, this provides the i965 implementation.

I've gone through all patches in the series and I replied to patches
where I had comments.  Overall, the series look good and with the
comments addressed, I'm ready to give my Reviewed-by for the series.
I want to take a closer look at the atomics lowering in patches 49+,
but I'm done for today.  Base on the quick look-through I did, I don't
expect to find any showstoppers there.

Here's a summary of what I found:

[PATCH v5 01/70] mesa: set MAX_SHADER_STORAGE_BUFFERS to 15.

  Update limit to 16 and drop the comment

[PATCH v5 02/70] i965: Use 16-byte offset alignment for shader storage buffers

  ctx->Const.ShaderStorageBufferOffsetAlignment should be 64

[PATCH v5 23/70] glsl: refactor parser processing of an interface block definition

  Clarify that the commit is just moving code.

[PATCH v5 26/70] glsl: Add parser/compiler support for std430 interface packing qualifier

  Update the error to also mention shader storage blocks, not just ubos?

[PATCH v5 28/70] glsl: add std430 interface packing support to ssbo related operations

  Why are we passing false for is_std430 here (emit_access in
  handle_rvalue)?  We use handle_rvalue for both UBO and SSBO loads,
  right?  Also, for consistency, I'd prefer if we could just pass
  'packing' around instead of is_std430.

[PATCH v5 29/70] glsl: a shader storage buffer must be smaller than the maximum size allowed

  Two chunks look like they should be their own patch ("Add unsized
  array support to glsl_type::std140_size" or such).

[PATCH v5 38/70] i965/nir/vec4: Implement nir_intrinsic_store_ssbo

  Shouldn't this be 'skipped_channels += num_channels;' to handle write mask reg.yw?

[PATCH v5 40/70] nir: Implement __intrinsic_load_ssbo

  Refactor handling of cmp instruction for converting to bool

[PATCH v5 54/70] glsl: First argument to atomic functions must be a buffer variable

  Nitpick: move check that only looks at first element in list to after loop

Also, I expect that before we land this series (thought that shouldn't
be far off), we'll have deleted the vec4 GLSL IR visitor so we can
drop these patches (I didn't review them):

[PATCH v5 16/70] i965/vec4: Implement ir_unop_get_buffer_size
[PATCH v5 39/70] i965/vec4: Implement __intrinsic_store_ssbo
[PATCH v5 43/70] i965/vec4: Implement __intrinsic_load_ssbo
[PATCH v5 53/70] i965/vec4: Implement lowered SSBO atomic intrinsics

I wrote the initial prototype of the SSBO functionality, but I don't
recall writing:

[PATCH v5 45/70] glsl: atomic counters can be declared as buffer-qualified variables

I don't think I did anything for atomics in my patch. Feel free to
take ownership of that one and add my Reviewed-by.

thanks,
Kristian