[Mesa-dev] [PATCH 14/36] glsl ubo/ssbo: Use enum to track current buffer access type

[Iago Toral]

On Mon, 2015-11-16 at 16:50 -0800, Jordan Justen wrote:
> On 2015-11-16 03:06:37, Iago Toral wrote:
> > On Sat, 2015-11-14 at 13:43 -0800, Jordan Justen wrote:
> > > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > > Cc: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
> > > Cc: Iago Toral Quiroga <itoral at igalia.com>
> > > ---
> > >  src/glsl/lower_ubo_reference.cpp | 26 +++++++++++++++++++++-----
> > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/glsl/lower_ubo_reference.cpp b/src/glsl/lower_ubo_reference.cpp
> > > index b74aa3d..41012db 100644
> > > --- a/src/glsl/lower_ubo_reference.cpp
> > > +++ b/src/glsl/lower_ubo_reference.cpp
> > > @@ -162,6 +162,14 @@ public:
> > >     ir_call *ssbo_store(ir_rvalue *deref, ir_rvalue *offset,
> > >                         unsigned write_mask);
> > >  
> > > +   enum {
> > > +      ubo_load_access,
> > > +      ssbo_load_access,
> > > +      ssbo_store_access,
> > > +      ssbo_get_array_length,
> > 
> > ssbo_get_array_length misses that is for "unsized" arrays and does not
> > include the "access" prefix that the other enum values have, which makes
> > it a bit inconsistent. How about we name this
> > 'ssbo_unsized_array_length_access'? or maybe 'ssbo_unsized_array_access'
> > if we think the former is too long.
> > 
> > > +      ssbo_atomic_access,
> > > +   } buffer_access_type;
> > > +
> > >     void emit_access(bool is_write, ir_dereference *deref,
> > >                      ir_variable *base_offset, unsigned int deref_offset,
> > >                      bool row_major, int matrix_columns,
> > > @@ -189,7 +197,6 @@ public:
> > >     struct gl_uniform_buffer_variable *ubo_var;
> > >     ir_rvalue *uniform_block;
> > >     bool progress;
> > > -   bool is_shader_storage;
> > >  };
> > >  
> > >  /**
> > > @@ -339,10 +346,9 @@ lower_ubo_reference_visitor::setup_for_load_or_store(ir_variable *var,
> > >                             deref, &nonconst_block_index);
> > >  
> > >     /* Locate the block by interface name */
> > > -   this->is_shader_storage = var->is_in_shader_storage_block();
> > >     unsigned num_blocks;
> > >     struct gl_uniform_block **blocks;
> > > -   if (this->is_shader_storage) {
> > > +   if (buffer_access_type != ubo_load_access) {
> > 
> > I think this file generally uses 'this->' to refer to class members (or
> > at least this function does), so maybe we should keep that for
> > consistency. The same in the other places where you use
> > buffer_access_type.
> 
> I don't really agree with this, but I went ahead and changed it.
> 
> > That said, right now it seems that we only ever use buffer_access_type
> > here and you always assign its value right before calling
> > setup_for_load_or_store() so maybe it is better to just make it a
> > function parameter instead of a class member? setup_for_load_or_store()
> > already has a large number of parameters, so I am not super happy about
> > the idea, but it looks more natural to me. What do you think?
> 
> This function is going to move to
> lower_buffer_access::setup_buffer_access. This class doesn't know
> about enum of buffer_access_type, since that remains part of the
> lower_ubo_reference_visitor class.
> 
> Therefore, I think we need to keep it as a member variable, so the
> insert_buffer_access virtual function implementation in
> lower_ubo_reference_visitor can make use of it.

We could define the buffer_access_type enum in the header file of
lower_buffer_access so we can use it from both lower_ubo_reference.cpp
and lower_buffer_access, so it should be possible, if we think it is a
good idea. In any case, setup_buffer_access has a lot of parameters
already... so feel free to ignore this comment if you like your
implementation better.

With the other changes I suggested,
Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

> -Jordan
> 
> > 
> > >        num_blocks = shader->NumShaderStorageBlocks;
> > >        blocks = shader->ShaderStorageBlocks;
> > >     } else {
> > > @@ -552,6 +558,10 @@ lower_ubo_reference_visitor::handle_rvalue(ir_rvalue **rvalue)
> > >     int matrix_columns;
> > >     unsigned packing = var->get_interface_type()->interface_packing;
> > >  
> > > +   buffer_access_type =
> > > +      var->is_in_shader_storage_block() ?
> > > +      ssbo_load_access : ubo_load_access;
> > > +
> > >     /* Compute the offset to the start if the dereference as well as other
> > >      * information we need to configure the write
> > >      */
> > > @@ -795,7 +805,7 @@ lower_ubo_reference_visitor::emit_access(bool is_write,
> > >        if (is_write)
> > >           base_ir->insert_after(ssbo_store(deref, offset, write_mask));
> > >        else {
> > > -         if (!this->is_shader_storage) {
> > > +         if (buffer_access_type == ubo_load_access) {
> > >               base_ir->insert_before(assign(deref->clone(mem_ctx, NULL),
> > >                                             ubo_load(deref->type, offset)));
> > >           } else {
> > > @@ -862,7 +872,7 @@ lower_ubo_reference_visitor::emit_access(bool is_write,
> > >  
> > >              base_ir->insert_after(ssbo_store(swizzle(deref, i, 1), chan_offset, 1));
> > >           } else {
> > > -            if (!this->is_shader_storage) {
> > > +            if (buffer_access_type == ubo_load_access) {
> > >                 base_ir->insert_before(assign(deref->clone(mem_ctx, NULL),
> > >                                               ubo_load(deref_type, chan_offset),
> > >                                               (1U << i)));
> > > @@ -891,6 +901,8 @@ lower_ubo_reference_visitor::write_to_memory(ir_dereference *deref,
> > >     int matrix_columns;
> > >     unsigned packing = var->get_interface_type()->interface_packing;
> > >  
> > > +   buffer_access_type = ssbo_store_access;
> > > +
> > >     /* Compute the offset to the start if the dereference as well as other
> > >      * information we need to configure the write
> > >      */
> > > @@ -1068,6 +1080,8 @@ lower_ubo_reference_visitor::process_ssbo_unsized_array_length(ir_rvalue **rvalu
> > >     unsigned packing = var->get_interface_type()->interface_packing;
> > >     int unsized_array_stride = calculate_unsized_array_stride(deref, packing);
> > >  
> > > +   buffer_access_type = ssbo_get_array_length;
> > > +
> > >     /* Compute the offset to the start if the dereference as well as other
> > >      * information we need to calculate the length.
> > >      */
> > > @@ -1181,6 +1195,8 @@ lower_ubo_reference_visitor::lower_ssbo_atomic_intrinsic(ir_call *ir)
> > >     int matrix_columns;
> > >     unsigned packing = var->get_interface_type()->interface_packing;
> > >  
> > > +   buffer_access_type = ssbo_atomic_access;
> > > +
> > >     setup_for_load_or_store(var, deref,
> > >                             &offset, &const_offset,
> > >                             &row_major, &matrix_columns,
> > 
> > 
>