[Mesa-dev] [PATCH 28/40] glsl/mesa: make a copy of attribute bindings in case of cache fallback

[Nicolai Hähnle]

On 07.02.2017 04:42, Timothy Arceri wrote:
> From: Timothy Arceri <timothy.arceri at collabora.com>
>
> If the shader cache falls back to doing a compile and link we need the
> original attribute bindings as they could have changed after the program
> was first linked.

So, same question as for patch #22, I don't really understand why the 
fallback attributes should be necessary and what the data flow is, since 
the cache_fallback happens during glLinkProgram().

On top of that:

> ---
>  src/compiler/glsl/shader_cache.cpp | 15 +++++++++++++++
>  src/mesa/main/mtypes.h             |  7 +++++++
>  src/mesa/main/shaderobj.c          |  6 ++++++
>  3 files changed, 28 insertions(+)
>
> diff --git a/src/compiler/glsl/shader_cache.cpp b/src/compiler/glsl/shader_cache.cpp
> index 729dd09..ddcd530 100644
> --- a/src/compiler/glsl/shader_cache.cpp
> +++ b/src/compiler/glsl/shader_cache.cpp
> @@ -808,9 +808,22 @@ read_hash_table(struct blob_reader *metadata, struct string_to_uint_map *hash)
>  }
>
>  static void
> +copy_hash_entry(const void *key, void *data, void *closure)
> +{
> +   struct string_to_uint_map *ht = (struct string_to_uint_map *) closure;
> +
> +   /* string_to_uint_map increases everything by 1 so we need to allow for
> +    * this when copying the data directly.
> +    */
> +   ht->put(((intptr_t) data) - 1, (const char *) key);
> +}
> +
> +static void
>  write_hash_tables(struct blob *metadata, struct gl_shader_program *prog)
>  {
>     write_hash_table(metadata, prog->AttributeBindings);
> +   hash_table_call_foreach(prog->AttributeBindings->ht, copy_hash_entry,
> +                           prog->data->FallbackAttributeBindings);
>     write_hash_table(metadata, prog->FragDataBindings);
>     write_hash_table(metadata, prog->FragDataIndexBindings);
>  }
> @@ -819,6 +832,8 @@ static void
>  read_hash_tables(struct blob_reader *metadata, struct gl_shader_program *prog)
>  {
>     read_hash_table(metadata, prog->AttributeBindings);
> +   hash_table_call_foreach(prog->AttributeBindings->ht, copy_hash_entry,
> +                           prog->data->FallbackAttributeBindings);

It seems rather suspicious that both write_hash_tables and 
read_hash_tables perform the copy in the same direction, and I don't see 
FallbackAttributeBindings being used anywhere else.

Cheers,
Nicolai

>     read_hash_table(metadata, prog->FragDataBindings);
>     read_hash_table(metadata, prog->FragDataIndexBindings);
>  }
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index f65cd76..d1dde0c 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -2688,6 +2688,13 @@ struct gl_shader_program_data
>     GLuint NumFallbackShaders;
>     struct gl_shader **FallbackShaders; /**< Shaders used for cache fallback */
>
> +   /**
> +    * If the shader cache falls back to doing a compile and link we need the
> +    * original attribute bindings as they could have changed after the program
> +    * was first linked.
> +    */
> +   struct string_to_uint_map *FallbackAttributeBindings;
> +
>     /** List of all active resources after linking. */
>     struct gl_program_resource *ProgramResourceList;
>     unsigned NumProgramResourceList;
> diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
> index ed19a72..245a0b9 100644
> --- a/src/mesa/main/shaderobj.c
> +++ b/src/mesa/main/shaderobj.c
> @@ -276,6 +276,7 @@ init_shader_program(struct gl_shader_program *prog)
>     prog->RefCount = 1;
>
>     prog->AttributeBindings = string_to_uint_map_ctor();
> +   prog->data->FallbackAttributeBindings = string_to_uint_map_ctor();
>     prog->FragDataBindings = string_to_uint_map_ctor();
>     prog->FragDataIndexBindings = string_to_uint_map_ctor();
>
> @@ -393,6 +394,11 @@ _mesa_free_shader_program_data(struct gl_context *ctx,
>        shProg->AttributeBindings = NULL;
>     }
>
> +   if (shProg->data->FallbackAttributeBindings) {
> +      string_to_uint_map_dtor(shProg->data->FallbackAttributeBindings);
> +      shProg->data->FallbackAttributeBindings = NULL;
> +   }
> +
>     if (shProg->FragDataBindings) {
>        string_to_uint_map_dtor(shProg->FragDataBindings);
>        shProg->FragDataBindings = NULL;
>