[Mesa-dev] [PATCH 5/6] glsl: apply shader storage block member rules when adding program resources

[Timothy Arceri]

On Fri, 2015-09-25 at 10:24 +0200, Samuel Iglesias Gonsalvez wrote:
> From ARB_program_interface_query:
> 
> "For an active shader storage block member declared as an array, an
>  entry will be generated only for the first array element, regardless
>  of its type. For arrays of aggregate types, the enumeration rules
> are
>  applied recursively for the single enumerated array element."
> 
> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
> ---
>  src/glsl/linker.cpp | 56
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index be04f5b..8cc9350 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -3134,6 +3134,57 @@ check_explicit_uniform_locations(struct
> gl_context *ctx,
>  }
>  
>  static bool
> +should_add_buffer_variable(struct gl_shader_program *shProg,
> +                           GLenum type, const char *name)
> +{
> +   bool found_interface = false;
> +   const char *block_name = NULL;
> +
> +   assert(type == GL_BUFFER_VARIABLE);
> +
> +   for (unsigned i = 0; i < shProg->NumUniformShaderStorageBlocks;
> i++) {
> +      block_name = shProg->UniformBlocks[i].Name;
> +      if (strncmp(block_name, name, strlen(block_name)) == 0) {
> +         found_interface = true;
> +         break;
> +      }
> +   }
> +
> +   /* We remove the interface name from the buffer variable name,
> +    * including the dot that follows it.
> +    */
> +   if (found_interface)
> +      name = name + strlen(block_name) + 1;
> +
> +   /* From: ARB_program_interface_query extension:
> +    *
> +    *  "For an active shader storage block member declared as an
> array, an
> +    *   entry will be generated only for the first array element,
> regardless
> +    *   of its type.  For arrays of aggregate types, the enumeration
> rules are
> +    *   applied recursively for the single enumerated array element.
> +    */
> +   const char *first_dot = strchr(name, '.');
> +   const char *first_square_bracket = strchr(name, '[');
> +
> +   /* The buffer variable is on top level and it is not an array or
> struct */
> +   if (!first_square_bracket && !first_dot) {
> +      return true;
> +   /* The shader storage block member is a struct, then generate the
> entry */
> +   } else if ((!first_square_bracket ||
> +               (first_dot && first_dot < first_square_bracket))) {

I think the above can be simplified to:

   if (!first_square_bracket) {
      return true;
   /* The shader storage block member is a struct, then generate the
entry */
   } else if (first_dot && first_dot < first_square_bracket)) {

> +      return true;
> +   } else {
> +      /* Shader storage block member is an array, only generate an
> entry for the
> +       * first array element.
> +       */
> +      if (strncmp(first_square_bracket, "[0]", 3) == 0)
> +         return true;
> +   }
> +
> +   return false;
> +}
> +
> +static bool
>  add_program_resource(struct gl_shader_program *prog, GLenum type,
>                       const void *data, uint8_t stages)
>  {
> @@ -3408,6 +3459,11 @@ build_program_resource_list(struct
> gl_shader_program *shProg)
>  
>        bool is_shader_storage =  shProg
> ->UniformStorage[i].is_shader_storage;
>        GLenum type = is_shader_storage ? GL_BUFFER_VARIABLE :
> GL_UNIFORM;
> +      if (is_shader_storage &&
> +          !should_add_buffer_variable(shProg, type,
> +                                      shProg
> ->UniformStorage[i].name))

Rather than check is_shader_storage here it would be better to replace
the assert at the top of should_add_buffer_variable() with:

   if (type != GL_BUFFER_VARIABLE)
      return false;

> +         continue;
> +
>        if (!add_program_resource(shProg, type,
>                                  &shProg->UniformStorage[i],
> stageref))
>           return;