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

[Iago Toral]

Hi Kristian,

On Fri, 2015-09-18 at 16:56 -0700, Kristian Høgsberg wrote:
> 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.

great, thanks for reviewing this! We will send new versions of the
patches for which you had comments or reply to your comments otherwise.

Iago

> 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
>