[Mesa-dev] [PATCH] glsl: use var with initializer on global var validation

[Kenneth Graunke]

On Friday, May 6, 2016 10:55:36 AM PDT Juan A. Suarez Romero wrote:
> Currently, when cross validating global variables, all global variables
> seen in the shaders that are part of a program are saved in a table.
> 
> When checking a variable this already exist in the table, we check both
> are initialized to the same value. If the already saved variable does
> not have an initializer, we copy it from the new variable.
> 
> Unfortunately this is wrong, as we are modifying something it is
> constant. Also, if this modified variable is used in
> another program, it will keep the initializer, when it should have none.
> 
> Instead of copying the initializer, this commit replaces the old
> variable with the new one. So if we see again the same variable with an
> initializer, we can compare if both are the same or not.
> ---
>  src/compiler/glsl/glsl_symbol_table.cpp | 10 ++++++++++
>  src/compiler/glsl/glsl_symbol_table.h   |  5 +++++
>  src/compiler/glsl/linker.cpp            | 27 +++++----------------------
>  3 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/src/compiler/glsl/glsl_symbol_table.cpp b/src/compiler/glsl/
glsl_symbol_table.cpp
> index 6c682ac..6d7baad 100644
> --- a/src/compiler/glsl/glsl_symbol_table.cpp
> +++ b/src/compiler/glsl/glsl_symbol_table.cpp
> @@ -278,3 +278,13 @@ glsl_symbol_table::disable_variable(const char *name)
>        entry->v = NULL;
>     }
>  }
> +
> +void
> +glsl_symbol_table::replace_variable(const char *name,
> +                                    ir_variable *v)
> +{
> +   symbol_table_entry *entry = get_entry(name);
> +   if (entry != NULL) {
> +      entry->v = v;
> +   }
> +}
> diff --git a/src/compiler/glsl/glsl_symbol_table.h b/src/compiler/glsl/
glsl_symbol_table.h
> index 5d654e5..2f94d4c 100644
> --- a/src/compiler/glsl/glsl_symbol_table.h
> +++ b/src/compiler/glsl/glsl_symbol_table.h
> @@ -100,6 +100,11 @@ struct glsl_symbol_table {
>      */
>     void disable_variable(const char *name);
>  
> +   /**
> +    * Replaces the variable in the entry by the new variable.
> +    */
> +   void replace_variable(const char *name, ir_variable *v);
> +
>  private:
>     symbol_table_entry *get_entry(const char *name);
>  
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 9c72478..4522734 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -1093,21 +1093,11 @@ cross_validate_globals(struct gl_shader_program 
*prog,
>  		     return;
>  		  }
>  	       } else {
> -		  /* If the first-seen instance of a particular uniform did not
> -		   * have an initializer but a later instance does, copy the
> -		   * initializer to the version stored in the symbol table.
> -		   */
> -		  /* FINISHME: This is wrong.  The constant_value field should
> -		   * FINISHME: not be modified!  Imagine a case where a shader
> -		   * FINISHME: without an initializer is linked in two different
> -		   * FINISHME: programs with shaders that have differing
> -		   * FINISHME: initializers.  Linking with the first will
> -		   * FINISHME: modify the shader, and linking with the second
> -		   * FINISHME: will fail.
> -		   */
> -		  existing->constant_initializer =
> -		     var->constant_initializer->clone(ralloc_parent(existing),
> -						      NULL);
> +		  /* If the first-seen instance of a particular uniform did
> +		   * not have an initializer but a later instance does,
> +		   * replace the former with the later.
> +                   */

Please convert the tabs in the above lines to spaces.

Thanks for fixing this!

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +                  variables.replace_variable(existing->name, var);
>  	       }
>  	    }
>  
> @@ -1121,13 +1111,6 @@ cross_validate_globals(struct gl_shader_program 
*prog,
>  			       var->name);
>  		  return;
>  	       }
> -
> -	       /* Some instance had an initializer, so keep track of that.  In
> -		* this location, all sorts of initializers (constant or
> -		* otherwise) will propagate the existence to the variable
> -		* stored in the symbol table.
> -		*/
> -	       existing->data.has_initializer = true;
>  	    }
>  
>  	    if (existing->data.invariant != var->data.invariant) {
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160510/48e282ad/attachment.sig>