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

[Iago Toral]

On Wed, 2015-09-23 at 10:38 -0700, Kristian Høgsberg wrote:
> On Wed, Sep 23, 2015 at 12:06 AM, Samuel Iglesias Gonsálvez
> <siglesias at igalia.com> wrote:
> >
> >
> > On 19/09/15 01:56, 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.
> >>
> >> 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.
> >>
> >
> > OK, I will take the ownership.
> 
> Cool.  With the update of patch 40, the entire series looks good to me:
> 
> Reviewed-by: Kristian Høgsberg <krh at bitplanet.net>
> 
> I think we should try to land this before the end of the week. With
> Marks Jenkins run showing only an couple of issues that have now been
> addressed, I don't see any reason to not merge this. We're early in
> the release cycle and it's a good time to land this and give it some
> exposure.

Great! we will push this later today.
Thanks to everyone for all the reviews!

Iago

> thanks,
> Kristian
> 
> > Thanks for the review,
> >
> > Sam
> >
> >> thanks,
> >> Kristian
> >>
>