[Mesa-dev] [PATCH] glsl: mark explicit uniforms as explicit in other stages too

[Timothy Arceri]

On Thu, 2016-01-14 at 13:02 +0200, Tapani Pälli wrote:
> If shader declares uniform explicit location in one stage but
> implicit in
> another, explicit location should be used. Patch marks implicit
> uniforms
> as explicit if they were explicit in another stage. This makes sure
> that
> we don't treat them implicit later when assigning locations.


I think it would be better to fix the code in cross_validate_globals()
it seems it is halfway doing this already but it only copies the
explicit location from later stages into the first uniform it found,
and not the otherway around 

I think you just need to check if the existing uniform is explicit and
copy the location to the currently matched var if its not explicit and
it should fix this bug.

> 
> Fixes following CTS test:
>    ES31-CTS.explicit_uniform_location.uniform-loc-implicit-in-some
> -stages3
> 
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
>  src/glsl/linker.cpp | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 564c471..70e701c 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -3131,11 +3131,33 @@ check_explicit_uniform_locations(struct
> gl_context *ctx,
>      */
>     string_to_uint_map *uniform_map = new string_to_uint_map;
>  
> -   if (!uniform_map) {
> +   /* This map is used to validate that uniforms with explicit
> location are
> +    * given the same explicit location properly across shader stages
> which
> +    * reference the same uniform without explicit location
> qualifier.
> +    */
> +   string_to_uint_map *explicit_map = new string_to_uint_map;
> +
> +   if (!uniform_map || !explicit_map) {
>        linker_error(prog, "Out of memory during linking.\n");
>        return;
>     }
>  
> +   /* A pass to generate hash that contains all uniforms where
> location
> +    * has been set explicitly.
> +    */
> +   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
> +      struct gl_shader *sh = prog->_LinkedShaders[i];
> +      if (!sh)
> +         continue;
> +      foreach_in_list(ir_instruction, node, sh->ir) {
> +         ir_variable *var = node->as_variable();
> +         if (!var || var->data.mode != ir_var_uniform ||
> +             !var->data.explicit_location)
> +            continue;
> +         explicit_map->put(var->data.location, var->name);
> +      }
> +   }
> +
>     unsigned entries_total = 0;
>     for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>        struct gl_shader *sh = prog->_LinkedShaders[i];
> @@ -3148,6 +3170,17 @@ check_explicit_uniform_locations(struct
> gl_context *ctx,
>           if (!var || var->data.mode != ir_var_uniform)
>              continue;
>  
> +         /* Check if this uniform with implicit location was marked
> explicit
> +          * by any shader stage. If so, mark it explicit in this
> stage too to
> +          * make sure later processing does not treat it as implicit
> one.
> +          */
> +         unsigned hash_loc;
> +         if (!var->data.explicit_location &&
> +             explicit_map->get(hash_loc, var->name)) {
> +            var->data.explicit_location = 1;
> +            var->data.location = hash_loc;
> +         }
> +
>           entries_total += var->type->uniform_locations();
>  
>           if (var->data.explicit_location) {
> @@ -3173,6 +3206,7 @@ check_explicit_uniform_locations(struct
> gl_context *ctx,
>                     ctx->Const.MaxUserAssignableUniformLocations);
>     }
>     delete uniform_map;
> +   delete explicit_map;
>  }
>  
>  static bool