[Mesa-dev] [PATCH] glsl: propagate max_array_access through function calls

[Ian Romanick]

On 08/21/2013 05:57 PM, Dominik Behr wrote:
> Fixes a bug where if an uniform array is passed to a function the accesses
> to the array are not propagated so later all but the first vector of the
> uniform array are removed in parcel_out_uniform_storage resulting in
> broken shaders and out of bounds access to arrays in
> brw::vec4_visitor::pack_uniform_registers.
>
> Signed-off-by: Dominik Behr <dbehr at chromium.org>
> ---
>   src/glsl/link_functions.cpp | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>
> diff --git a/src/glsl/link_functions.cpp b/src/glsl/link_functions.cpp
> index 6b3e154..32e2012 100644
> --- a/src/glsl/link_functions.cpp
> +++ b/src/glsl/link_functions.cpp
> @@ -173,6 +173,32 @@ public:
>         return visit_continue;
>      }
>
> +   virtual ir_visitor_status visit_leave(ir_call *ir)
> +   {
> +      /* traverse list of function parameters, and for array parameters
> +         propagate max_array_access, do it when leaving the node
> +         so the childen would propagate their array accesses first */
                    children

I also think this comment should be expanded.  On the first read of this 
code, it wasn't obvious to me why this was necessary.  It's worth 
mentioning that the important case is when the reference to the array at 
the call site does not reference a specific element.  In that case the 
access range of the from the callee is propagated back to the array 
passed in.

> +      exec_list_iterator sig_param_iter = ir->callee->parameters.iterator();
> +      exec_list_iterator param_iter = ir->actual_parameters.iterator();

While iterators are a fine design pattern, the implementation of them in 
the compiler front-end is garbage (and I made that implementation).  We 
stopped writing new code to use the iterators years ago.  There is other 
code that does a similar double-walk of the signature parameters and 
actual parameters lists.

I think the best example is in 
lower_clip_distance_visitor::visit_leave(ir_call *ir) in 
lower_clip_distance.cpp.

> +      while (param_iter.has_next()) {
> +         ir_variable *sig_param = (ir_variable *) sig_param_iter.get();
> +         ir_rvalue *param = (ir_rvalue *) param_iter.get();
> +         assert(sig_param);
> +         assert(param);
> +         if (sig_param->type->is_array()) {
> +            ir_dereference_variable *deref = param->as_dereference_variable();
> +            if (deref && deref->var && deref->var->type->is_array()) {
> +               if (sig_param->max_array_access > deref->var->max_array_access) {
> +                   deref->var->max_array_access = sig_param->max_array_access;
> +               }

This should end up looking more similar to the code in 
call_link_visitor::visit(ir_dereference_variable *ir) around line 200 
(without your patch).

            var->max_array_access =
                MAX2(var->max_array_access, ir->var->max_array_access);

> +            }
> +         }
> +         sig_param_iter.next();
> +         param_iter.next();
> +      }
> +      return visit_continue;
> +   }
> +
>      virtual ir_visitor_status visit(ir_dereference_variable *ir)
>      {
>         if (hash_table_find(locals, ir->var) == NULL) {
>