[Mesa-dev] [PATCH v2] main: buffer array variables can have array size of 0 if they are unsized

[Timothy Arceri]

On Tue, 2015-10-06 at 15:07 +0200, Samuel Iglesias Gonsálvez wrote:
> On 06/10/15 12:59, Timothy Arceri wrote:
> > On Tue, 2015-10-06 at 10:08 +0200, Samuel Iglesias Gonsalvez wrote:
> > > From ARB_program_query_interface:
> > > 
> > >   For the property ARRAY_SIZE, a single integer identifying the
> > > number of
> > >   active array elements of an active variable is written to
> > > <params>.
> > > The
> > >   array size returned is in units of the type associated with the
> > > property
> > >   TYPE. For active variables not corresponding to an array of
> > > basic
> > > types,
> > >   the value one is written to <params>. If the variable is a
> > > shader
> > >   storage block member in an array with no declared size, the
> > > value
> > > zero
> > >   is written to <params>.
> > > 
> > > v2:
> > > 
> > > - Unsized arrays of arrays have an array size different than zero
> > > 
> > > Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
> > > ---
> > >  src/mesa/main/shader_query.cpp | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/mesa/main/shader_query.cpp
> > > b/src/mesa/main/shader_query.cpp
> > > index dfc39f6..17076b8 100644
> > > --- a/src/mesa/main/shader_query.cpp
> > > +++ b/src/mesa/main/shader_query.cpp
> > > @@ -1304,8 +1304,12 @@ _mesa_program_resource_prop(struct
> > > gl_shader_program *shProg,
> > >        switch (res->Type) {
> > >        case GL_UNIFORM:
> > >        case GL_BUFFER_VARIABLE:
> > > +         if (RESOURCE_UNI(res)->is_shader_storage &&
> > > +             RESOURCE_UNI(res)->is_unsized_array)
> > > +            *val = RESOURCE_UNI(res)->array_elements;
> > 
> > You don't need RESOURCE_UNI(res)->is_unsized_array here anymore
> > right? 
> > 
> 
> I still needed it here: this rule only applies to unsized arrays. The
> rest of variables need the MAX2(...) because active variables not
> corresponding to an array of basic types should write the value one
> in
> '*val'.

Right I forgot about types not being an array. My concern is adding yet
another field to the uniform storage struct. IMO it already contains
too many fields, since this is something that hangs around in memory
for every uniform we should probably make at least some effort to keep
it as small as possible.

I haven't tested it but I think you could use (RESOURCE_UNI(res)
->array_stride > 0) as a test of whether the buffer is an array. So
here you could have:

  if (RESOURCE_UNI(res)->is_shader_storage &&                   
      RESOURCE_UNI(res)->array_stride > 0)

And in the patch you linked to [0] you would have:

   case GL_BUFFER_VARIABLE:
      if (RESOURCE_UNI(res)->array_stride > 0 &&
          RESOURCE_UNI(res)->array_elements == 0)
         return 1;
      else
         return RESOURCE_UNI(res)->array_elements;

You also might want some comments to go with it but what do you think
about this approach?

> 
> If I remove it from here (just for testing), hundreds of dEQP GLES31
> tests start to fail
> (dEQP-GLES31.functional.ssbo.layout.single_basic_type.*.*).
> 
> RESOURCE_UNI(res)->is_unsized_array is also needed in "main: consider
> that unsized arrays have at least one active element" [0].
> 
> Sam
> 
> [0] http://lists.freedesktop.org/archives/mesa-dev/2015-October/09603
> 8.html
> 
> > RESOURCE_UNI(res)->is_shader_storage is all you need so you can
> > also
> > drop the patch that adds is_unsized_array to the uniform storage
> > struct
> > 
> > With that change: Reviewed-by: Timothy Arceri <
> > t_arceri at yahoo.com.au>
> > 
> > 
> > > +         else
> > >              *val = MAX2(RESOURCE_UNI(res)->array_elements, 1);
> > > -            return 1;
> > > +         return 1;
> > >        case GL_PROGRAM_INPUT:
> > >        case GL_PROGRAM_OUTPUT:
> > >           *val = MAX2(_mesa_program_resource_array_size(res), 1);
> >