[Mesa-dev] [PATCH 21/22] i965: Setup glsl uniforms by index rather than name matching

[Timothy Arceri]

On 18/04/18 00:36, Alejandro Piñeiro wrote:
> From: Neil Roberts <nroberts at igalia.com>
> 
> Previously when setting up a uniform it would try to walk the uniform
> storage slots and find one that matches the name of the given
> variable. However, each variable already has a location which is an
> index into the UniformStorage array so we can just directly jump to
> the right slot. Some of the variables take up more than one slot so we
> still need to calculate how many it uses.
> 
> The main reason to do this is to support ARB_gl_spirv because in that
> case the uniforms don’t have names so the previous approach won’t
> work.

This is a nice improvement away :)

However it might be nicer to just update the existing 
glsl_type::uniform_locations() helper
rather than create the custom count_uniform_storage_slots() helper.

e.g

unsigned
glsl_type::uniform_locations(bool uniform_storage_locs) const
{
    unsigned size = 0;

    switch (this->base_type) {
    case GLSL_TYPE_UINT:
    case GLSL_TYPE_INT:
    case GLSL_TYPE_FLOAT:
    case GLSL_TYPE_FLOAT16:
    case GLSL_TYPE_DOUBLE:
    case GLSL_TYPE_UINT16:
    case GLSL_TYPE_UINT8:
    case GLSL_TYPE_INT16:
    case GLSL_TYPE_INT8:
    case GLSL_TYPE_UINT64:
    case GLSL_TYPE_INT64:
    case GLSL_TYPE_BOOL:
    case GLSL_TYPE_SAMPLER:
    case GLSL_TYPE_IMAGE:
    case GLSL_TYPE_SUBROUTINE:
       return 1;

    case GLSL_TYPE_STRUCT:
    case GLSL_TYPE_INTERFACE:
       for (unsigned i = 0; i < this->length; i++)
          size += this->fields.structure[i].type->uniform_locations();
       return size;
    case GLSL_TYPE_ARRAY:
       if (!uniform_storage_locs ||
           this->fields.array->is_array() ||
           this->fields.array->is_interface() ||
           this->fields.array->is_record()) {
           /* For uniform locations passed to the user via the API we
            * count all array elements.
            */
          return this->length * this->fields.array->uniform_locations();
       } else {
          /* For uniform storage the innermost array only uses a
           * single slot.
           */
          return 1;
       }
    default:
       return 0;
    }
}

If you agree this patch is:

Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>

> ---
>   src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 50 +++++++++++++++++++-------
>   1 file changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> index 62b2951432a..cb5e07f74ba 100644
> --- a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> @@ -118,35 +118,59 @@ brw_setup_image_uniform_values(gl_shader_stage stage,
>      }
>   }
>   
> +static unsigned
> +count_uniform_storage_slots(const struct glsl_type *type)
> +{
> +   /* gl_uniform_storage can cope with one level of array, so if the
> +    * type is a composite type or an array where each element occupies
> +    * more than one slot than we need to recursively process it.
> +    */
> +   if (glsl_type_is_struct(type)) {
> +      unsigned location_count = 0;
> +
> +      for (unsigned i = 0; i < glsl_get_length(type); i++) {
> +         const struct glsl_type *field_type = glsl_get_struct_field(type, i);
> +
> +         location_count += count_uniform_storage_slots(field_type);
> +      }
> +
> +      return location_count;
> +   }
> +
> +   if (glsl_type_is_array(type)) {
> +      const struct glsl_type *element_type = glsl_get_array_element(type);
> +
> +      if (glsl_type_is_array(element_type) ||
> +          glsl_type_is_struct(element_type)) {
> +         unsigned element_count = count_uniform_storage_slots(element_type);
> +         return element_count * glsl_get_length(type);
> +      }
> +   }
> +
> +   return 1;
> +}
> +
>   static void
>   brw_nir_setup_glsl_uniform(gl_shader_stage stage, nir_variable *var,
>                              const struct gl_program *prog,
>                              struct brw_stage_prog_data *stage_prog_data,
>                              bool is_scalar)
>   {
> -   int namelen = strlen(var->name);
> -
>      /* The data for our (non-builtin) uniforms is stored in a series of
>       * gl_uniform_storage structs for each subcomponent that
>       * glGetUniformLocation() could name.  We know it's been set up in the same
> -    * order we'd walk the type, so walk the list of storage and find anything
> -    * with our name, or the prefix of a component that starts with our name.
> +    * order we'd walk the type, so walk the list of storage that matches the
> +    * range of slots covered by this variable.
>       */
>      unsigned uniform_index = var->data.driver_location / 4;
> -   for (unsigned u = 0; u < prog->sh.data->NumUniformStorage; u++) {
> +   unsigned num_slots = count_uniform_storage_slots(var->type);
> +   for (unsigned u = 0; u < num_slots; u++) {
>         struct gl_uniform_storage *storage =
> -         &prog->sh.data->UniformStorage[u];
> +         &prog->sh.data->UniformStorage[var->data.location + u];
>   
>         if (storage->builtin || storage->type->is_sampler())
>            continue;
>   
> -      if (strncmp(var->name, storage->name, namelen) != 0 ||
> -          (storage->name[namelen] != 0 &&
> -           storage->name[namelen] != '.' &&
> -           storage->name[namelen] != '[')) {
> -         continue;
> -      }
> -
>         if (storage->type->is_image()) {
>            brw_setup_image_uniform_values(stage, stage_prog_data,
>                                           uniform_index, storage);
>