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 > <sigles...@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 <k...@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 > >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev