[Mesa-dev] [PATCH 03/18] nir/linker: use empty block info to assign uniform locations

[Timothy Arceri]

On 30/06/18 00:28, Alejandro Piñeiro wrote:
> For the cases of uniforms that doesn't have an explicit
> location. Under ARB_gl_spirv those are exceptions, like uniform atomic
> counters.
> ---
>   src/compiler/glsl/gl_nir_link_uniforms.c | 31 +++++++++++++++++--------------
>   1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c
> index 388c1ab63fc..77d3eaa5f2b 100644
> --- a/src/compiler/glsl/gl_nir_link_uniforms.c
> +++ b/src/compiler/glsl/gl_nir_link_uniforms.c
> @@ -79,6 +79,8 @@ nir_setup_uniform_remap_tables(struct gl_context *ctx,
>      }
>   
>      /* Reserve locations for rest of the uniforms. */
> +   link_util_update_empty_uniform_locations(prog);
> +
>      for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
>         struct gl_uniform_storage *uniform = &prog->data->UniformStorage[i];
>   
> @@ -93,22 +95,23 @@ nir_setup_uniform_remap_tables(struct gl_context *ctx,
>         if (uniform->remap_location != UNMAPPED_UNIFORM_LOC)
>            continue;
>   
> -      /* How many new entries for this uniform? */
> +      /* How many 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.
> -       */
> -      unsigned chosen_location = prog->NumUniformRemapTable;
> -
> -      /* resize remap table to fit new entries */
> -      prog->UniformRemapTable =
> -         reralloc(prog,
> -                  prog->UniformRemapTable,
> -                  struct gl_uniform_storage *,
> -                  prog->NumUniformRemapTable + entries);
> -      prog->NumUniformRemapTable += entries;
> +      unsigned chosen_location =

Please move  patch 2 to patch 1 and squash with this one with the 
current patch 1. This is just code churn.

As with my review of patch 1 please rename chosen_location -> location

Otherwise:

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


> +         link_util_find_empty_block(prog, &prog->data->UniformStorage[i]);
> +
> +      if (chosen_location == -1) {
> +         chosen_location = prog->NumUniformRemapTable;
> +
> +         /* 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;
>