[Mesa-dev] [PATCH 3/4] linker: Add uniform_field_visitor class to process leaf fields of a uniform

[Kenneth Graunke]

On 10/24/2011 02:16 PM, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
> 
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
>  src/glsl/Makefile          |    1 +
>  src/glsl/link_uniforms.cpp |  116 ++++++++++++++++++++++++++++++++++++++++++++
>  src/glsl/linker.h          |   45 +++++++++++++++++
>  3 files changed, 162 insertions(+), 0 deletions(-)
>  create mode 100644 src/glsl/link_uniforms.cpp
> 
> diff --git a/src/glsl/Makefile b/src/glsl/Makefile
> index b2efb2a..504f1fb 100644
> --- a/src/glsl/Makefile
> +++ b/src/glsl/Makefile
> @@ -53,6 +53,7 @@ CXX_SOURCES = \
>  	ir_variable_refcount.cpp \
>  	linker.cpp \
>  	link_functions.cpp \
> +	link_uniforms.cpp \
>  	loop_analysis.cpp \
>  	loop_controls.cpp \
>  	loop_unroll.cpp \
> diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> new file mode 100644
> index 0000000..38bc00c
> --- /dev/null
> +++ b/src/glsl/link_uniforms.cpp
> @@ -0,0 +1,116 @@
> +/*
> + * Copyright © 2011 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include "main/core.h"
> +#include "ir.h"
> +#include "linker.h"
> +#include "glsl_symbol_table.h"
> +#include "program/hash_table.h"
> +
> +/**
> + * \file link_uniforms.cpp
> + * Assign locations for GLSL uniforms.
> + *
> + * \author Ian Romanick <ian.d.romanick at intel.com>
> + */
> +
> +void uniform_field_visitor::process(ir_variable *var)
> +{
> +   char *name = const_cast<char *>(var->name);

I loathe const_cast<>---it's just evil.  I declare my variables const
for a reason, and I don't want people screwing them up.  Guess how fun
it is to debug issues caused by your supposedly const values changing
out from under you?

I see that you're trying to avoid a strdup in the common case, which is
good, but I'd prefer a less dirty solution.

> +   unsigned len = 0;
> +
> +   recursion(var->type, &name, strlen(name), len);
> +   if (name != var->name)
> +      free(name);
> +}
> +
> +void uniform_field_visitor::recursion(const glsl_type *t, char **name,
> +				      unsigned name_length,
> +				      unsigned &name_buffer_length)
> +{
> +   /* Records need to have each field processed individually.
> +    *
> +    * Arrays of records need to have each array element processed
> +    * individually, then each field of the resulting array elements processed
> +    * individually.
> +    */
> +   if (t->is_record()) {
> +      for (unsigned i = 0; i < t->length; i++) {
> +	 const unsigned len = strlen(t->fields.structure[i].name);
> +
> +	 /* Size of "name.field\0".
> +	  */
> +	 const unsigned field_length = name_length + 1 + len + 1;
> +
> +	 /* Grow the name buffer if it is not large enough to hold the fully
> +	  * qualified name of this field.
> +	  */
> +	 if (name_buffer_length < field_length) {
> +	    if (name_buffer_length == 0) {
> +	       name_buffer_length = field_length + 16;
> +	       char *tmp = (char *) malloc(name_buffer_length);
> +	       strcpy(tmp, *name);
> +
> +	       *name = tmp;
> +	    } else {
> +	       name_buffer_length = field_length + 16;
> +	       *name = (char *) realloc(*name, name_buffer_length);
> +	    }
> +	 }
> +
> +	 /* Append '.field' to the current uniform name.
> +	  */
> +	 (*name)[name_length] = '.';
> +	 memcpy(& (*name)[name_length+1], t->fields.structure[i].name, len);
> +	 (*name)[field_length - 1] = '\0';

Maybe it's just me, but I find this code rather impenetrable what with
all the buffer management and string munging.  mallocs, reallocs,
memcpys, strcpys, extra parameters, the magic constant + 16...

Yet, all it does is generate names such as
  some_struct.foo
  some_struct.bar
  some_struct.baz
and call a function for each of those.  That's really simple.

I understand that you're trying to avoid unnecessary strlen() calls, but
there has to be a better way.

I've added some new functionality to ralloc that should make this a lot
cleaner.  I'll send it out shortly.

> +	 recursion(t->fields.structure[i].type,
> +		   name, field_length - 1, name_buffer_length);
> +      }
> +   } else if (t->is_array() && t->fields.array->is_record()) {
> +      const unsigned extra = unsigned(ceil(log10(float(t->length)))) + 2;
> +      const unsigned field_length = name_length + extra;
> +
> +      if (name_buffer_length < field_length) {
> +	 if (name_buffer_length == 0) {
> +	    name_buffer_length = field_length + 16;
> +	    char *tmp = (char *) malloc(name_buffer_length);
> +	    strcpy(tmp, *name);
> +
> +	    *name = tmp;
> +	 } else {
> +	    name_buffer_length = field_length + 16;
> +	    *name = (char *) realloc(*name, name_buffer_length);
> +	 }
> +      }
> +
> +      for (unsigned i = 0; i < t->length; i++) {
> +	 const unsigned len = name_length
> +	    + sprintf(&(*name)[name_length], "[%d]", i);
> +
> +	 recursion(t->fields.array, name, len, name_buffer_length);
> +      }
> +   } else {
> +      this->visit_field(t, *name);
> +   }
> +}
> diff --git a/src/glsl/linker.h b/src/glsl/linker.h
> index 769cf68..2bd2b47 100644
> --- a/src/glsl/linker.h
> +++ b/src/glsl/linker.h
> @@ -1,3 +1,4 @@
> +/* -*- c++ -*- */
>  /*
>   * Copyright © 2010 Intel Corporation
>   *
> @@ -29,4 +30,48 @@ extern bool
>  link_function_calls(gl_shader_program *prog, gl_shader *main,
>  		    gl_shader **shader_list, unsigned num_shaders);
>  
> +
> +/**
> + * Class for processing all of the leaf fields of an uniform
> + *
> + * Leaves are, roughly speaking, the parts of the uniform that the application
> + * could query with \c glGetUniformLocation (or that could be returned by
> + * \c glGetActiveUniforms).
> + *
> + * Classes my derive from this class to implement specific functionality.
> + * This class only provides the mechanism to iterate over the leaves.  Derived
> + * classes must implement \c ::visit_field and may override \c ::process.
> + */
> +class uniform_field_visitor {
> +public:
> +   /**
> +    * Begin processing a uniform
> +    *
> +    * Classes that overload this function should call \c ::process from the
> +    * base class to start the recursive processing of the uniform.
> +    *
> +    * \param var  The uniform variable that is to be processed
> +    *
> +    * Calls \c ::visit_field for each leaf of the uniform.
> +    */
> +   void process(ir_variable *var);
> +
> +protected:
> +   /**
> +    * Method invoked for each leaf of the uniform
> +    *
> +    * \param type  Type of the field.
> +    * \param name  Fully qualified name of the field.
> +    */
> +   virtual void visit_field(const glsl_type *type, const char *name) = 0;
> +
> +private:
> +   /**
> +    * \param name_length  Length of the current name \b not including the
> +    *                     terminating \c NUL character.
> +    */
> +   void recursion(const glsl_type *t, char **name, unsigned name_length,
> +		  unsigned &name_buffer_length);
> +};
> +
>  #endif /* GLSL_LINKER_H */