[Mesa-dev] [PATCH 01/18] nir/linker: handle uniforms without explicit location

[Timothy Arceri]

On 30/06/18 16:05, Timothy Arceri wrote:
> On 30/06/18 00:28, Alejandro Piñeiro wrote:
>> ARB_gl_spirv points that uniforms in general need explicit
>> location. But there are still some cases of uniforms without location,
>> like for example uniform atomic counters. Those doesn't have a
>> location from the OpenGL point of view (they are identified with a
>> binding), but Mesa internally assigns it a location.
>>
>> Signed-off-by: Eduardo Lima <elima at igalia.com>
>> Signed-off-by: Alejandro Piñeiro <apinheiro at igalia.com>
>> Signed-off-by: Neil Roberts <nroberts at igalia.com>
>> ---
>>
>> The @FIXME included on the patch below is solved with the follow-up
>> path "nir/linker: use empty block info to assign uniform locations",
>> so perhaps it makes sense to just squash both patches. I don't have a
>> strong opinion on that, but I think that it would be easier to review
>> as splitted patches.
>>
>>
>>   src/compiler/glsl/gl_nir_link_uniforms.c | 61 
>> ++++++++++++++++++++++++++++++--
>>   1 file changed, 59 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c 
>> b/src/compiler/glsl/gl_nir_link_uniforms.c
>> index c6961fbb6ca..388c1ab63fc 100644
>> --- a/src/compiler/glsl/gl_nir_link_uniforms.c
>> +++ b/src/compiler/glsl/gl_nir_link_uniforms.c
>> @@ -36,6 +36,8 @@
>>    * normal uniforms as mandatory, and so on).
>>    */
>> +#define UNMAPPED_UNIFORM_LOC ~0u
>> +
>>   static void
>>   nir_setup_uniform_remap_tables(struct gl_context *ctx,
>>                                  struct gl_shader_program *prog)
>> @@ -58,8 +60,59 @@ nir_setup_uniform_remap_tables(struct gl_context *ctx,
>>      for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
>>         struct gl_uniform_storage *uniform = 
>> &prog->data->UniformStorage[i];
>> +      if (prog->data->UniformStorage[i].remap_location == 
>> UNMAPPED_UNIFORM_LOC)
>> +         continue;
>> +
>> +      /* How many new entries for this uniform? */
>> +      const unsigned entries = MAX2(1, uniform->array_elements);
>> +      unsigned num_slots = glsl_get_component_slots(uniform->type);
>> +
>> +      uniform->storage = &data[data_pos];
>> +
>> +      /* Set remap table entries point to correct gl_uniform_storage. */
>> +      for (unsigned j = 0; j < entries; j++) {
>> +         unsigned element_loc = uniform->remap_location + j;
>> +         prog->UniformRemapTable[element_loc] = uniform;
>> +
>> +         data_pos += num_slots;
>> +      }
>> +   }
>> +
>> +   /* Reserve locations for rest of the uniforms. */
>> +   for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
>> +      struct gl_uniform_storage *uniform = 
>> &prog->data->UniformStorage[i];
>> +
>> +      if (uniform->is_shader_storage)
>> +         continue;
>> +
>> +      /* Built-in uniforms should not get any location. */
>> +      if (uniform->builtin)
>> +         continue;
>> +
>> +      /* Explicit ones have been set already. */
>> +      if (uniform->remap_location != UNMAPPED_UNIFORM_LOC)
>> +         continue;
>> +
>>         /* How many new entries for this uniform? */
>>         const unsigned entries = MAX2(1, uniform->array_elements);
>> +
>> +      /* @FIXME: By now, we add un-assigned uniform locations to the 
>> end of
>> +       * the uniform file. We need to keep track of empty locations 
>> and use
>> +       * them.
>> +       */
> 
> Maybe reword this to:
> 
> /* @FIXME: For now, we add un-assigned uniform locations to the end of
>   * the uniform file. We should fix this to keep track of empty
>   * locations and use those first.
>   */
> 
> 
>> +      unsigned chosen_location = prog->NumUniformRemapTable;
> 
> Can we please just rename chosen_location 

Whoops that should be:

Can we please just rename chosen_location -> location

> With those two nits this patch is:
> 
> Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>