[Mesa-dev] [PATCH 05/10] glsl/linker: assign explicit uniform locations

[Tapani]

On 05/19/2014 08:07 PM, Ian Romanick wrote:
> On 04/09/2014 02:56 AM, Tapani Pälli wrote:
>> Patch refactors the existing uniform processing so explicit locations
>> are taken in to account during variable processing. These locations
>> are temporarily stored in gl_uniform_storage before actual locations
>> are set.
>>
>> The 'remap_location' variable in gl_uniform_storage is changed to be
>> signed so that we can use 0 as a valid explicit location and '-1' as
>> identifier that no explicit location has been defined.
>>
>> When locations are set, UniformRemapTable is first populated with
>> uniforms that have explicit location set (inactive and actives ones),
>> rest are put after explicit location slots.
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>>   src/glsl/ir_uniform.h      |  5 +++--
>>   src/glsl/link_uniforms.cpp | 56 +++++++++++++++++++++++++++++++++++++++++-----
>>   2 files changed, 54 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/glsl/ir_uniform.h b/src/glsl/ir_uniform.h
>> index 3508509..9dc4a8e 100644
>> --- a/src/glsl/ir_uniform.h
>> +++ b/src/glsl/ir_uniform.h
>> @@ -181,9 +181,10 @@ struct gl_uniform_storage {
>>   
>>      /**
>>       * The 'base location' for this uniform in the uniform remap table. For
>> -    * arrays this is the first element in the array.
>> +    * arrays this is the first element in the array. It needs to be signed
>> +    * so that we can use 0 as valid location and -1 as initial value
>>       */
>> -   unsigned remap_location;
>> +   int remap_location;
> You could use ~0u instead of -1, right?  A #define for the magic value
> will also help.

Sure, I can move to using ~0u. Should be enough to never become a problem.

>>   };
>>   
>>   #ifdef __cplusplus
>> diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
>> index 29dc0b1..0f99082 100644
>> --- a/src/glsl/link_uniforms.cpp
>> +++ b/src/glsl/link_uniforms.cpp
>> @@ -387,6 +387,9 @@ public:
>>      void set_and_process(struct gl_shader_program *prog,
>>   			ir_variable *var)
>>      {
>> +      current_var = var;
>> +      field_counter = 0;
>> +
>>         ubo_block_index = -1;
>>         if (var->is_in_uniform_block()) {
>>            if (var->is_interface_instance() && var->type->is_array()) {
>> @@ -543,6 +546,22 @@ private:
>>            return;
>>         }
>>   
>> +      /* Assign explicit locations. */
>> +      if (current_var->data.explicit_location) {
>> +         /* Set sequential locations for struct fields. */
>> +         if (current_var->type->is_record()) {
> I think you can accomplish the same thing with record_type != NULL.

ok, I can change

>> +            const unsigned entries = MAX2(1, this->uniforms[id].array_elements);
>> +            this->uniforms[id].remap_location =
>> +               current_var->data.location + field_counter;
>> +               field_counter += entries;
> Weird indentation.

will fix

>> +         } else {
>> +            this->uniforms[id].remap_location = current_var->data.location;
>> +         }
>> +      } else {
>> +         /* Initialize to -1 to indicate that no explicit location is set */
>> +         this->uniforms[id].remap_location = -1;
>> +      }
>> +
>>         this->uniforms[id].name = ralloc_strdup(this->uniforms, name);
>>         this->uniforms[id].type = base_type;
>>         this->uniforms[id].initialized = 0;
>> @@ -598,6 +617,17 @@ public:
>>      gl_texture_index targets[MAX_SAMPLERS];
>>   
>>      /**
>> +    * Current variable being processed.
>> +    */
>> +   ir_variable *current_var;
>> +
>> +   /**
>> +    * Field counter is used to take care that uniform structures
>> +    * with explicit locations get sequential locations.
>> +    */
>> +   unsigned field_counter;
>> +
>> +   /**
>>       * Mask of samplers used by the current shader stage.
>>       */
>>      unsigned shader_samplers_used;
>> @@ -799,10 +829,6 @@ link_assign_uniform_locations(struct gl_shader_program *prog)
>>      prog->UniformStorage = NULL;
>>      prog->NumUserUniformStorage = 0;
>>   
>> -   ralloc_free(prog->UniformRemapTable);
>> -   prog->UniformRemapTable = NULL;
>> -   prog->NumUniformRemapTable = 0;
>> -
>>      if (prog->UniformHash != NULL) {
>>         prog->UniformHash->clear();
>>      } else {
>> @@ -915,9 +941,29 @@ link_assign_uniform_locations(struct gl_shader_program *prog)
>>                sizeof(prog->_LinkedShaders[i]->SamplerTargets));
>>      }
>>   
>> -   /* Build the uniform remap table that is used to set/get uniform locations */
>> +   /* Reserve all the explicit locations of the active uniforms. */
>> +   for (unsigned i = 0; i < num_user_uniforms; i++) {
>> +      if (uniforms[i].remap_location != -1) {
>> +         /* How many new entries for this uniform? */
>> +         const unsigned entries = MAX2(1, uniforms[i].array_elements);
>> +
>> +         /* Set remap table entries point to correct gl_uniform_storage. */
>> +         for (unsigned j = 0; j < entries; j++) {
>> +            unsigned element_loc = uniforms[i].remap_location + j;
>> +            assert(prog->UniformRemapTable[element_loc] ==
>> +                   (gl_uniform_storage *) -1);
>> +            prog->UniformRemapTable[element_loc] = &uniforms[i];
>> +         }
>> +      }
>> +   }
>> +
>> +   /* Reserve locations for rest of the uniforms. */
>>      for (unsigned i = 0; i < num_user_uniforms; i++) {
>>   
>> +      /* Explicit ones have been set already. */
>> +      if (uniforms[i].remap_location != -1)
>> +         continue;
>> +
>>         /* how many new entries for this uniform? */
>>         const unsigned entries = MAX2(1, uniforms[i].array_elements);
>>   
>>