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

[Neil Roberts]

Thanks for the feedback.

I’ve implemented your suggestion as the top two patches on a branch
here:

https://github.com/Igalia/mesa/tree/nroberts/count-uniform-storage

It also fixes a slight bug that it wasn’t passing down the
uniform_storage_locs argument into the recursion.

However I think my preference would be for keeping this as a separate
function because it seems a little confusing to make the function count two
different things depending on a bool argument. Perhaps for the sake of
maintainability it would be good to move it to glsl_types though.

If there is a strong preference to merge it into one function then of
course I don’t mind.

Regards,
- Neil

Timothy Arceri <tarceri at itsqueeze.com> writes:

> 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);
>> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180503/47a40c05/attachment.sig>