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

[Timothy Arceri]

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

With those two nits this patch is:

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

> +
> +      /* resize remap table to fit new entries */
> +      prog->UniformRemapTable =
> +         reralloc(prog,
> +                  prog->UniformRemapTable,
> +                  struct gl_uniform_storage *,
> +                  prog->NumUniformRemapTable + entries);
> +      prog->NumUniformRemapTable += entries;
> +
> +      /* set the base location in remap table for the uniform */
> +      uniform->remap_location = chosen_location;
> +
>         unsigned num_slots = glsl_get_component_slots(uniform->type);
>   
>         uniform->storage = &data[data_pos];
> @@ -302,8 +355,12 @@ nir_link_uniform(struct gl_context *ctx,
>         }
>         uniform->active_shader_mask |= 1 << stage;
>   
> -      /* Uniform has an explicit location */
> -      uniform->remap_location = location;
> +      if (location >= 0) {
> +         /* Uniform has an explicit location */
> +         uniform->remap_location = location;
> +      } else {
> +         uniform->remap_location = UNMAPPED_UNIFORM_LOC;
> +      }
>   
>         /* @FIXME: the initialization of the following will be done as we
>          * implement support for their specific features, like SSBO, atomics,
>