[Mesa-dev] [PATCH 8/8] glsl: Modify ir_set_program_inouts to handle geometry shaders.

[Ian Romanick]

A couple of comments below.  Other than that,

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

On 07/31/2013 02:18 PM, Paul Berry wrote:
> ---
>   src/glsl/ir_set_program_inouts.cpp | 85 ++++++++++++++++++++++++++++++++------
>   1 file changed, 73 insertions(+), 12 deletions(-)
>
> diff --git a/src/glsl/ir_set_program_inouts.cpp b/src/glsl/ir_set_program_inouts.cpp
> index 018342b..8f96324 100644
> --- a/src/glsl/ir_set_program_inouts.cpp
> +++ b/src/glsl/ir_set_program_inouts.cpp
> @@ -114,7 +114,13 @@ mark(struct gl_program *prog, ir_variable *var, int offset, int len,
>   void
>   ir_set_program_inouts_visitor::mark_whole_variable(ir_variable *var)
>   {
> -   mark(this->prog, var, 0, var->type->count_attribute_slots(),
> +   const glsl_type *type = var->type;
> +   if (this->shader_type == GL_GEOMETRY_SHADER &&
> +       var->mode == ir_var_shader_in && type->is_array()) {
> +      type = type->fields.array;
> +   }
> +
> +   mark(this->prog, var, 0, type->count_attribute_slots(),
>           this->shader_type == GL_FRAGMENT_SHADER);
>   }
>
> @@ -133,7 +139,9 @@ ir_set_program_inouts_visitor::visit(ir_dereference_variable *ir)
>   /**
>    * Try to mark a portion of the given variable as used.  Caller must ensure
>    * that the variable represents a shader input or output which can be indexed
> - * into in array fashion (an array, matrix, or vector).
> + * into in array fashion (an array, matrix, or vector).  For the purpose of
> + * geometry shader inputs (which are always arrays), this means that the array
> + * element must be something that can be indexed into in array fashion.

Except gl_PrimitiveIDIn, as noted below.

>    *
>    * If the index can't be interpreted as a constant, or some other problem
>    * occurs, then nothing will be marked and false will be returned.
> @@ -144,6 +152,16 @@ ir_set_program_inouts_visitor::try_mark_partial_variable(ir_variable *var,
>   {
>      const glsl_type *type = var->type;
>
> +   if (this->shader_type == GL_GEOMETRY_SHADER &&
> +       var->mode == ir_var_shader_in) {
> +      /* The only geometry shader input that is not an array is
> +       * gl_PrimitiveIDIn, and in that case, this code will never be reached,
> +       * because gl_PrimitiveIDIn can't be indexed into in array fashion.
> +       */
> +      assert(type->is_array());
> +      type = type->fields.array;
> +   }
> +
>      /* The code below only handles:
>       *
>       * - Indexing into matrices
> @@ -205,17 +223,60 @@ ir_set_program_inouts_visitor::try_mark_partial_variable(ir_variable *var,
>   ir_visitor_status
>   ir_set_program_inouts_visitor::visit_enter(ir_dereference_array *ir)
>   {
> -   ir_dereference_variable *deref_var;
> -   deref_var = ir->array->as_dereference_variable();
> -   ir_variable *var = deref_var ? deref_var->var : NULL;
> -
> -   /* Check that we're dereferencing a shader in or out */
> -   if (!var || !is_shader_inout(var))
> -      return visit_continue;
> -
> -   if (try_mark_partial_variable(var, ir->array_index))
> -      return visit_continue_with_parent;
> +   /* Note: for geometry shader inputs, lower_named_interface_blocks may
> +    * create 2D arrays, so we need to be able to handle those.  2D arrays
> +    * shouldn't be able to crop up for any other reason.
> +    */
> +   if (ir_dereference_array *inner_array =
> +       ir->array->as_dereference_array()) {

'ir_dereference_array *const'?

I'm not sure whether I love or hate this style.  I seem some significant 
advantages to it (the big one being the limited scope of the variable), 
but it has some stylistic issues (the long if-condition).  Having an 
assignment in an if-condition also feels icky.

I guess it's pretty similar to what we already do with for-loops, so I'm 
leaning towards liking it.

> +      /*          ir => foo[i][j]
> +       * inner_array => foo[i]
> +       */
> +      if (ir_dereference_variable *deref_var =
> +          inner_array->as_dereference_variable()) {
> +         if (this->shader_type == GL_GEOMETRY_SHADER &&
> +             deref_var->var->mode == ir_var_shader_in) {
> +            /* foo is a geometry shader input, so i is the vertex, and j the
> +             * part of the input we're accessing.
> +             */
> +            if (try_mark_partial_variable(deref_var->var, ir->array_index))
> +            {
> +               /* We've now taken care of foo and j, but i might contain a
> +                * subexpression that accesses shader inputs.  So manually
> +                * visit i and then continue with the parent.
> +                */
> +               inner_array->array_index->accept(this);
> +               return visit_continue_with_parent;
> +            }
> +         }
> +      }
> +   } else if (ir_dereference_variable *deref_var =
> +              ir->as_dereference_variable()) {
> +      /* ir => foo[i], where foo is a variable. */
> +      if (this->shader_type == GL_GEOMETRY_SHADER &&
> +          deref_var->var->mode == ir_var_shader_in) {
> +         /* foo is a geometry shader input, so i is the vertex, and we're
> +          * accessing the entire input.
> +          */
> +         mark_whole_variable(deref_var->var);
> +         /* We've now taken care of foo, but i might contain a subexpression
> +          * that accesses shader inputs.  So manually visit i and then
> +          * continue with the parent.
> +          */
> +         ir->array_index->accept(this);
> +         return visit_continue_with_parent;
> +      } else if (is_shader_inout(deref_var->var)) {
> +         /* foo is a shader input/output, but not a geometry shader input,
> +          * so i is the part of the input we're accessing.
> +          */
> +         if (try_mark_partial_variable(deref_var->var, ir->array_index))
> +            return visit_continue_with_parent;
> +      }
> +   }
>
> +   /* The expression is something we don't recognize.  Just visit its
> +    * subexpressions.
> +    */
>      return visit_continue;
>   }
>
>