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-sta...@lists.freedesktop.org
> > Signed-off-by: Kenneth Graunke <kenn...@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.arc...@collabora.com>
Thanks!
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
