[Mesa-dev] [PATCH 2/6] glsl: Split arrays even in the presence of whole-array copies.

[Kenneth Graunke]

On Wednesday, June 22, 2016 2:23:00 PM PDT Timothy Arceri wrote:
> On Tue, 2016-06-21 at 20:02 -0700, Kenneth Graunke wrote:
> > Previously, we failed to split constant arrays.  Code such as
> > 
> >    int[2] numbers = int[](1, 2);
> > 
> > would generates a whole-array assignment:
> > 
> >   (assign () (var_ref numbers)
> >              (constant (array int 4) (constant int 1) (constant int
> > 2)))
> > 
> > opt_array_splitting generally tried to visit ir_dereference_array
> > nodes,
> > and avoid recursing into the inner ir_dereference_variable.  So if it
> > ever saw a ir_dereference_variable, it assumed this was a whole-array
> > read and bailed.  However, in the above case, there's no array deref,
> > and we can totally handle it - we just have to "unroll" the
> > assignment,
> > creating assignments for each element.
> > 
> > This was mitigated by the fact that we constant propagate whole
> > arrays,
> > so a dereference of a single component would usually get the desired
> > single value anyway.  However, I plan to stop doing that shortly;
> > early experiments with disabling constant propagation of arrays
> > revealed this shortcoming.
> > 
> > This patch causes some arrays in Gl32GSCloth's geometry shaders to be
> > split, which allows other optimizations to eliminate unused GS
> > inputs.
> > The VS then doesn't have to write them, which eliminates the entire
> > VS
> > (5 -> 2 instructions).  It still renders correctly.
> > 
> > No other change in shader-db.
> > 
> > Cc: mesa-stable at lists.freedesktop.org
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/compiler/glsl/opt_array_splitting.cpp | 56
> > +++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/src/compiler/glsl/opt_array_splitting.cpp
> > b/src/compiler/glsl/opt_array_splitting.cpp
> > index a294da5..9faeb87 100644
> > --- a/src/compiler/glsl/opt_array_splitting.cpp
> > +++ b/src/compiler/glsl/opt_array_splitting.cpp
> > @@ -93,6 +93,7 @@ public:
> >     {
> >        this->mem_ctx = ralloc_context(NULL);
> >        this->variable_list.make_empty();
> > +      this->in_whole_array_copy = false;
> >     }
> >  
> >     ~ir_array_reference_visitor(void)
> > @@ -104,6 +105,8 @@ public:
> >  
> >     virtual ir_visitor_status visit(ir_variable *);
> >     virtual ir_visitor_status visit(ir_dereference_variable *);
> > +   virtual ir_visitor_status visit_enter(ir_assignment *);
> > +   virtual ir_visitor_status visit_leave(ir_assignment *);
> >     virtual ir_visitor_status visit_enter(ir_dereference_array *);
> >     virtual ir_visitor_status visit_enter(ir_function_signature *);
> >  
> > @@ -113,6 +116,8 @@ public:
> >     exec_list variable_list;
> >  
> >     void *mem_ctx;
> > +
> > +   bool in_whole_array_copy;
> >  };
> >  
> >  } /* namespace */
> > @@ -158,10 +163,34 @@ ir_array_reference_visitor::visit(ir_variable
> > *ir)
> >  }
> >  
> >  ir_visitor_status
> > +ir_array_reference_visitor::visit_enter(ir_assignment *ir)
> > +{
> > +   in_whole_array_copy =
> > +      ir->lhs->type->is_array() && !ir->lhs->type-
> > >is_array_of_arrays() &&
> > +      ir->whole_variable_written();
> 
> Maybe a TODO for AoA support? I assume we would just need to do some
> kind of recersive call in the new code below or is there more to it? If
> there is more to it?

Heh, good point - I mostly didn't want to think about it, and originally
didn't have code to handle it.  But I added the recursive call (the
assign_i->accept()) while fixing bugs.  It seems like it should work,
as we just unroll & split one level of arrays.  Jenkins is happy.

So I'll drop the !AOA check.  Thanks!

> 
> > +
> > +   return visit_continue;
> > +}
> > +
> > +ir_visitor_status
> > +ir_array_reference_visitor::visit_leave(ir_assignment *ir)
> > +{
> > +   in_whole_array_copy = false;
> > +
> > +   return visit_continue;
> > +}
> > +
> > +ir_visitor_status
> >  ir_array_reference_visitor::visit(ir_dereference_variable *ir)
> >  {
> >     variable_entry *entry = this->get_variable_entry(ir->var);
> >  
> > +   /* Ignore whole-array assignments on the LHS.  We can split those
> > +    * by "unrolling" the assignment into component-wise assignments.
> > +    */
> 
> Instead of ignore maybe "Allow" or "Allow splitting of" or something
> like that I think makes it easier to understand whats going on.

Yeah, that's much better.  I've changed it to "Allow".

> Otherwise patch 1-2 are:
> 
> Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>

Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160622/144d98b4/attachment-0001.sig>