[Mesa-dev] [PATCH] glsl: Update deref types when resizing implicitly sized arrays.

[Timothy Arceri]

On Wed, 2016-11-02 at 14:28 -0700, Kenneth Graunke wrote:
> At link time, we resolve the size of implicitly sized arrays.
> When doing so, we update the type of the ir_variables.  However,
> we neglected to update the type of ir_dereference nodes which
> reference those variables.
> 
> It turns out array_resize_visitor (for GS/TCS/TES interface array
> handling) already did 2/3 of the cases for this, so we can simply
> refactor the code and reuse it.
> 
> This fixes:
> GL45-CTS.shader_storage_buffer_object.basic-syntax
> GL45-CTS.shader_storage_buffer_object.basic-syntaxSSO
> 
> which have an SSBO containing an implicitly sized array, followed
> by some other members.  setup_buffer_access uses the dereference
> types to compute offsets to fields, and it had a stale type where
> the implicitly sized array's length was still 0 instead of the
> actual length.
> 
> While we're here, we can also fix update_array_sizes to properly
> update deref types as well, fixing a FINISHME from 2010.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/compiler/glsl/linker.cpp | 70 +++++++++++++++++++++++++++++-----
> ----------
>  1 file changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/src/compiler/glsl/linker.cpp
> b/src/compiler/glsl/linker.cpp
> index 03b866f..bd650a6 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -181,7 +181,43 @@ private:
>  };
>  
>  
> -class array_resize_visitor : public ir_hierarchical_visitor {
> +/**
> + * A visitor helper that provides methods for updating the types of
> + * ir_dereferences.  Classes that update variable types (say,
> updating
> + * array sizes) will want to use this so that dereference types stay
> in sync.
> + */
> +class deref_type_updater : public ir_hierarchical_visitor {
> +public:
> +   virtual ir_visitor_status visit(ir_dereference_variable *ir)
> +   {
> +      ir->type = ir->var->type;
> +      return visit_continue;
> +   }
> +
> +   virtual ir_visitor_status visit_leave(ir_dereference_array *ir)
> +   {
> +      const glsl_type *const vt = ir->array->type;
> +      if (vt->is_array())
> +         ir->type = vt->fields.array;
> +      return visit_continue;
> +   }
> +
> +   virtual ir_visitor_status visit_leave(ir_dereference_record *ir)
> +   {
> +      for (unsigned i = 0; i < ir->record->type->length; i++) {
> +         const struct glsl_struct_field *field =
> +            &ir->record->type->fields.structure[i];
> +         if (strcmp(field->name, ir->field) == 0) {
> +            ir->type = field->type;
> +            break;
> +         }
> +      }
> +      return visit_continue;
> +   }
> +};
> +
> +
> +class array_resize_visitor : public deref_type_updater {
>  public:
>     unsigned num_vertices;
>     gl_shader_program *prog;
> @@ -240,24 +276,6 @@ public:
>  
>        return visit_continue;
>     }
> -
> -   /* Dereferences of input variables need to be updated so that
> their type
> -    * matches the newly assigned type of the variable they are
> accessing. */
> -   virtual ir_visitor_status visit(ir_dereference_variable *ir)
> -   {
> -      ir->type = ir->var->type;
> -      return visit_continue;
> -   }
> -
> -   /* Dereferences of 2D input arrays need to be updated so that
> their type
> -    * matches the newly assigned type of the array they are
> accessing. */
> -   virtual ir_visitor_status visit_leave(ir_dereference_array *ir)
> -   {
> -      const glsl_type *const vt = ir->array->type;
> -      if (vt->is_array())
> -         ir->type = vt->fields.array;
> -      return visit_continue;
> -   }
>  };
>  
>  /**
> @@ -1361,7 +1379,7 @@ move_non_declarations(exec_list *instructions,
> exec_node *last,
>   * it inside that function leads to compiler warnings with some
> versions of
>   * gcc.
>   */
> -class array_sizing_visitor : public ir_hierarchical_visitor {
> +class array_sizing_visitor : public deref_type_updater {
>  public:
>     array_sizing_visitor()
>        : mem_ctx(ralloc_context(NULL)),
> @@ -2283,6 +2301,8 @@ update_array_sizes(struct gl_shader_program
> *prog)
>           if (prog->_LinkedShaders[i] == NULL)
>              continue;
>  
> +      bool updated_any_types = false;

I'm probably being picky but IMO it would read better just as
types_updated.

With or without the change:

Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>


> +
>        foreach_in_list(ir_instruction, node, prog->_LinkedShaders[i]-
> >ir) {
>           ir_variable *const var = node->as_variable();
>  
> @@ -2338,11 +2358,15 @@ update_array_sizes(struct gl_shader_program
> *prog)
>  
>              var->type = glsl_type::get_array_instance(var->type-
> >fields.array,
>                                                        size + 1);
> -            /* FINISHME: We should update the types of array
> -             * dereferences of this variable now.
> -             */
> +            updated_any_types = true;
>           }
>        }
> +
> +      /* Update the types of dereferences in case we changed any. */
> +      if (updated_any_types) {
> +         deref_type_updater v;
> +         v.run(prog->_LinkedShaders[i]->ir);
> +      }
>     }
>  }
>