[Mesa-dev] [PATCH v2] glsl: fix variable ordering in the output_read_remover

[Ian Romanick]

On 04/04/2012 07:33 AM, Vadim Girlin wrote:
> Use the hash of the variable name instead of the pointer value.
>
> Signed-off-by: Vadim Girlin<vadimgirlin at gmail.com>
> ---
>
> v2: Added the comment for the hash function as suggested by Vincent Lejeune.
>      Added const qualifier and modified the cast as suggested by Tolga Dalman.
>
>   src/glsl/lower_output_reads.cpp |   15 ++++++++++++++-
>   1 files changed, 14 insertions(+), 1 deletions(-)
>
> diff --git a/src/glsl/lower_output_reads.cpp b/src/glsl/lower_output_reads.cpp
> index 415b541..0427f0d 100644
> --- a/src/glsl/lower_output_reads.cpp
> +++ b/src/glsl/lower_output_reads.cpp
> @@ -54,11 +54,24 @@ public:
>      virtual ir_visitor_status visit_leave(class ir_function_signature *);
>   };
>
> +/**
> + * Hash function for the output variables - computes the hash of the name.
> + * NOTE: We're using the name string to ensure that the hash doesn't depend
> + * on any random factors, otherwise the output_read_remover could produce
> + * the random order of the assignments.
> + */
> +static unsigned
> +hash_table_var_hash(const void *key)
> +{
> +   const ir_variable * var = static_cast<const ir_variable *>(key);
> +   return hash_table_string_hash(var->name);

We don't usually use the name as the hashkey because you can have 
multiple distinct variables with the same name.  These can be variables 
from different scopes or compiler-generated names (though we try to 
generate unique names in that case).  Since this pass only applies to 
shader outputs, I don't think this problem can occur.  Shader outputs 
should have unique names.

My main concern is that someone may see this code in the future and copy 
it in a situation where it is not safe.  With the addition of some 
comment that describes this,

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> +}
> +
>   output_read_remover::output_read_remover()
>   {
>      mem_ctx = ralloc_context(NULL);
>      replacements =
> -      hash_table_ctor(0, hash_table_pointer_hash, hash_table_pointer_compare);
> +      hash_table_ctor(0, hash_table_var_hash, hash_table_pointer_compare);
>   }
>
>   output_read_remover::~output_read_remover()