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

[Dominik Behr]

Thanks,
I looked at piglit tests and they look OK if they are only supposed to test
whether the shader compiles and links. It doesn't look like they test the
results of rendering which could be more useful?


On Tue, Sep 3, 2013 at 3:19 PM, Dominik Behr <dbehr at google.com> wrote:

> Thanks,
> I looked at piglit tests and they look OK if they are only supposed to
> test whether the shader compiles and links. It doesn't look like they test
> the results of rendering which could be more useful?
>
> --
> Dominik
>
>
> On Tue, Sep 3, 2013 at 2:52 PM, Matt Turner <mattst88 at gmail.com> wrote:
>
>> 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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130903/7ae9a3dd/attachment-0001.html>