[Mesa-dev] [PATCH v2 21/42] glsl: Add default matrix ordering in lower_buffer_access

[Jordan Justen]

On 2015-11-26 01:10:03, Iago Toral wrote:
> On Wed, 2015-11-25 at 11:59 -0800, Jordan Justen wrote:
> > On 2015-11-25 01:32:37, Iago Toral wrote:
> > > 
> > > On Tue, 2015-11-17 at 21:54 -0800, Jordan Justen wrote:
> > > > For compute shader shared variable we will set a default of column
> > > > major.
> > > > 
> > > > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > > > ---
> > > >  src/glsl/lower_buffer_access.cpp |  5 +++--
> > > >  src/glsl/lower_buffer_access.h   | 10 ++++++++++
> > > >  2 files changed, 13 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/glsl/lower_buffer_access.cpp b/src/glsl/lower_buffer_access.cpp
> > > > index 297ed69..66e7abe 100644
> > > > --- a/src/glsl/lower_buffer_access.cpp
> > > > +++ b/src/glsl/lower_buffer_access.cpp
> > > > @@ -281,8 +281,9 @@ lower_buffer_access::is_dereferenced_thing_row_major(const ir_rvalue *deref)
> > > >  
> > > >           switch (matrix_layout) {
> > > >           case GLSL_MATRIX_LAYOUT_INHERITED:
> > > > -            assert(!matrix);
> > > > -            return false;
> > > > +            assert(default_matrix_layout != GLSL_MATRIX_LAYOUT_INHERITED ||
> > > > +                   !matrix);
> > > > +            return default_matrix_layout == GLSL_MATRIX_LAYOUT_ROW_MAJOR;
> > > 
> > > I am not sure I understand this. If shared variables are column major by
> > > default, then isn't that the same behavior we have for ubos and ssbos?
> > > In what case is this needed?
> > 
> > I think some code must walk the interface blocks an assign the
> > interface matrix layouts to matrices which are set to inherit their
> > layout.
> 
> Yes, there is code for that in ast_to_hir.cpp, see
> ast_interface_block::hir().
> 
> > Since shared variables are not part of an interface block, this
> > doesn't happen for matrices in shared variables. This leads to us
> > hitting the previous assert(!matrix) code.
> 
> Ok, I see.
> 
> > Instead of this change we could also go through and mark all matrices
> > in shared variables with a layout other than 'inherited' at an earlier
> > stage.
> 
> Probably not worth it if we can handle it here without extra cost. Maybe
> we can make the code more obvious though.
> 
> How about this instead?:
> 
> case GLSL_MATRIX_LAYOUT_INHERITED: {
>    /* For interface block matrix variables we handle inherited layouts
>     * at HIR generation time, but we don't do that for shared 
>     * variables, which are always column-major
>     */
>    ir_variable *var = deref->variable_referenced();
>    assert((var->is_in_buffer_block() && !matrix) ||
>           var->data.mode == ir_var_shader_shared)
>    return false;
> }

Yeah, I like your idea better. Thanks!

-Jordan

> 
> > 
> > > 
> > > >           case GLSL_MATRIX_LAYOUT_COLUMN_MAJOR:
> > > >              return false;
> > > >           case GLSL_MATRIX_LAYOUT_ROW_MAJOR:
> > > > diff --git a/src/glsl/lower_buffer_access.h b/src/glsl/lower_buffer_access.h
> > > > index f8e1070..82b35ed 100644
> > > > --- a/src/glsl/lower_buffer_access.h
> > > > +++ b/src/glsl/lower_buffer_access.h
> > > > @@ -39,6 +39,14 @@ namespace lower_buffer_access {
> > > >  
> > > >  class lower_buffer_access : public ir_rvalue_enter_visitor {
> > > >  public:
> > > > +   lower_buffer_access() :
> > > > +      default_matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED)
> > > > +   {}
> > > > +
> > > > +   lower_buffer_access(enum glsl_matrix_layout default_matrix_layout) :
> > > > +      default_matrix_layout(default_matrix_layout)
> > > > +   {}
> > > > +
> > > >     virtual void
> > > >     insert_buffer_access(void *mem_ctx, ir_dereference *deref,
> > > >                          const glsl_type *type, ir_rvalue *offset,
> > > > @@ -55,6 +63,8 @@ public:
> > > >                              ir_rvalue **offset, unsigned *const_offset,
> > > >                              bool *row_major, int *matrix_columns,
> > > >                              unsigned packing);
> > > > +
> > > > +   enum glsl_matrix_layout default_matrix_layout;
> > > >  };
> > > >  
> > > >  } /* namespace lower_buffer_access */
> > > 
> > > 
> > > 
> > 
> 
>