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

[Ilia Mirkin]

I'm still unclear what problem you're trying to solve here? What's
wrong with having b[1] = b[1]?

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