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

[Matt Turner]

On Wed, Aug 28, 2013 at 1:10 PM, Dominik Behr <dbehr at chromium.org> 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 | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/src/glsl/link_functions.cpp b/src/glsl/link_functions.cpp
> index 6b3e154..d935546 100644
> --- a/src/glsl/link_functions.cpp
> +++ b/src/glsl/link_functions.cpp
> @@ -173,6 +173,35 @@ 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, Otherwise arrays that are only referenced
> +         from inside functions via function parameters will be incorrectly
> +         optimized. This will lead to incorrect code being generated (or worse).
> +         Do it when leaving the node so the childen would propagate their
> +         array accesses first */
> +
> +      const exec_node *formal_param_node = ir->callee->parameters.get_head();
> +      const exec_node *actual_param_node = ir->actual_parameters.get_head();
> +      while (!actual_param_node->is_tail_sentinel()) {
> +         ir_variable *formal_param = (ir_variable *) formal_param_node;
> +         ir_rvalue *actual_param = (ir_rvalue *) actual_param_node;
> +
> +         formal_param_node = formal_param_node->get_next();
> +         actual_param_node = actual_param_node->get_next();
> +
> +         if (formal_param->type->is_array()) {
> +            ir_dereference_variable *deref = actual_param->as_dereference_variable();
> +            if (deref && deref->var && deref->var->type->is_array()) {
> +               deref->var->max_array_access =
> +                  MAX2(formal_param->max_array_access, deref->var->max_array_access);
> +            }
> +         }
> +      }
> +      return visit_continue;
> +   }
> +
>     virtual ir_visitor_status visit(ir_dereference_variable *ir)
>     {
>        if (hash_table_find(locals, ir->var) == NULL) {
> --
> 1.8.3.1

Reviewed-and-Tested-by: Matt Turner <mattst88 at gmail.com>

I've sent four tests to the piglit list and Cc'd you. Take a look at
them and make sure they're exercising the thing you want to test.

I'll commit this patch tomorrow, assuming no other comments or
problems with the tests. I'll also tag it for the stable branches,
since it's definitely a bug fix.

Thanks a bunch, Dominik!

Matt