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

[Samuel Iglesias Gonsálvez]

On 28/09/15 00:05, Timothy Arceri wrote:
> 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)) {
> 

Yes, thanks.

>> +      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;
> 

Actually, it should return true, as we don't want to skip any uniform. I
will add this code to should_add_buffer_variable() together with a
comment to explain why it returns true.

Sam

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