On Mon, Sep 21, 2015 at 12:39:14PM +0200, Iago Toral wrote:
> 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.
I had a look at the atomics lowering and that looks fine. Except for
patch 40/70, which I hope we can clean up a little bit, all updated
patches and explanation in 38/70 look good.
Kristian
> 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
> >
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
