[Mesa-dev] [PATCH] glsl: remove redundant function inout assignments

[Timothy Arceri]

On Tue, 2015-11-03 at 19:21 -0500, Ilia Mirkin wrote:
> I'm still unclear what problem you're trying to solve here? What's
> wrong with having b[1] = b[1]?

There is nothing wrong with it, but nir seems to expect this type of
assignment to be optimised out and hits an assert if its not. Its removed for
non arrays in opt_copy_propagation.cpp see add_copy()

> 
> On Tue, Nov 3, 2015 at 7:19 PM, Timothy Arceri <t_arceri at yahoo.com.au>
> wrote:
> > On Mon, 2015-11-02 at 03:27 -0500, Ilia Mirkin wrote:
> > > On Sun, Nov 1, 2015 at 3:33 AM, Timothy Arceri <t_arceri at yahoo.com.au>
> > > wrote:
> > > > Handles the case with function inout params where array elements
> > > > do an assignment to themselves e.g.
> > > > 
> > > >  void array_mod(inout int b[2])
> > > >  {
> > > >     b[0] = int(2);
> > > >     b[1] = b[1];
> > > >  }
> > > > 
> > > >  Fixes assert in nir for:
> > > >  ES31-CTS.arrays_of_arrays.InteractionFunctionCalls2
> > > > 
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=92588
> > > > ---
> > > >  src/glsl/opt_copy_propagation_elements.cpp | 46
> > > > ++++++++++++++++++++++++++++++
> > > >  1 file changed, 46 insertions(+)
> > > > 
> > > >  Piglit test: https://patchwork.freedesktop.org/patch/63355/
> > > > 
> > > > diff --git a/src/glsl/opt_copy_propagation_elements.cpp
> > > > b/src/glsl/opt_copy_propagation_elements.cpp
> > > > index 353a5c6..a62b625 100644
> > > > --- a/src/glsl/opt_copy_propagation_elements.cpp
> > > > +++ b/src/glsl/opt_copy_propagation_elements.cpp
> > > > @@ -439,6 +439,8 @@
> > > > ir_copy_propagation_elements_visitor::kill(kill_entry
> > > > *k)
> > > >  /**
> > > >   * Adds directly-copied channels between vector variables to the
> > > > available
> > > >   * copy propagation list.
> > > > + *
> > > > + * Also tidy up redundant function inout assignments while we are
> > > > here.
> > > >   */
> > > >  void
> > > >  ir_copy_propagation_elements_visitor::add_copy(ir_assignment *ir)
> > > > @@ -450,6 +452,50 @@
> > > > ir_copy_propagation_elements_visitor::add_copy(ir_assignment *ir)
> > > >     if (ir->condition)
> > > >        return;
> > > > 
> > > > +   /* Handle a corner case with function inout params where array
> > > > elements
> > > > +    * do an assignment to themselves e.g.
> > > > +    *
> > > > +    * void array_mod(inout int b[2])
> > > > +    * {
> > > > +    *    b[0] = int(2);
> > > > +    *    b[1] = b[1];
> > > > +    * }
> > > 
> > > What if you have like
> > > 
> > > array_mod(inout vec2 b[2]) {
> > >   b[1] = b[1].xy;
> > 
> > This is handled by the change also. The swizzle will be removed before
> > getting
> > here.
> > 
> > > or
> > >   b[1] = b[1].yx;
> > 
> > This will fail the first rhs_array->as_dereference_array() check (as it
> > will
> > be a swizzle) so we will leave it alone as we should.
> > 
> > Just to be sure I wrote a couple of piglit tests and Cc'ed you in on them.
> > 
> > > }
> > > 
> > > IOW, why is this case so special?
> > > 
> > > > +    */
> > > > +   ir_rvalue *rhs_array = ir->rhs;
> > > > +   ir_rvalue *lhs_array = ir->lhs;
> > > > +   if (lhs_array->as_dereference_array() &&
> > > > +       rhs_array->as_dereference_array()) {
> > > > +      /* Check arrays are indexed by a const and match otherwise
> > > > return
> > > > */
> > > > +      while (rhs_array->as_dereference_array() &&
> > > > +             lhs_array->as_dereference_array()) {
> > > > +
> > > > +         ir_dereference_array *rhs_deref_array =
> > > > +            rhs_array->as_dereference_array();
> > > > +         ir_dereference_array *lhs_deref_array =
> > > > +            lhs_array->as_dereference_array();
> > > > +
> > > > +         ir_constant *lhs_ai_const =
> > > > +            lhs_deref_array->array_index->as_constant();
> > > > +         ir_constant *rhs_ai_const =
> > > > +            rhs_deref_array->array_index->as_constant();
> > > > +         if (lhs_ai_const == NULL || rhs_ai_const == NULL ||
> > > > +             lhs_ai_const->value.i[0] != rhs_ai_const->value.i[0])
> > > > +            return;
> > > > +         lhs_array = lhs_deref_array->array;
> > > > +         rhs_array = rhs_deref_array->array;
> > > > +      }
> > > > +
> > > > +      ir_dereference_variable *lhs_var = lhs_array
> > > > ->as_dereference_variable();
> > > > +      ir_dereference_variable *rhs_var = rhs_array
> > > > ->as_dereference_variable();
> > > > +      if(lhs_var && rhs_var && lhs_var->var == rhs_var->var){
> > > 
> > > spaces please
> > > 
> > > > +        /* Removing the assignment now would mess up the loop
> > > > iteration
> > > > +         * calling us.  Just flag it to not execute, and someone else
> > > > +         * will clean up the mess.
> > > > +         */
> > > > +        ir->condition = new(ralloc_parent(ir)) ir_constant(false);
> > > 
> > > I thought that the allocator was smart and you could just do new (ir)
> > > ir_constant(false). e.g.
> > > 
> > > src/glsl/lower_jumps.cpp:            new (ir) ir_constant(true)));
> > 
> > Yeah I think your right, this was a copy and paste from
> > opt_copy_propagation.cpp will chnage before pushing thanks.
> > 
> > > 
> > > > +      }
> > > > +   }
> > > > +
> > > >     ir_dereference_variable *lhs = ir->lhs->as_dereference_variable();
> > > >     if (!lhs || !(lhs->type->is_scalar() || lhs->type->is_vector()))
> > > >        return;
> > > > --
> > > > 2.4.3
> > > > 
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev