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

2016-06-22 Thread 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-sta...@lists.freedesktop.org
> > Signed-off-by: Kenneth Graunke 
> > ---
> >  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 

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

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

2016-06-21 Thread Timothy Arceri 
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 
> ---
>  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?

> +
> +   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.

Otherwise patch 1-2 are:

Reviewed-by: Timothy Arceri 

> +   if (in_assignee && in_whole_array_copy)
> +  return visit_continue;
> +
> /* If we made it to here without seeing an ir_dereference_array,
>  * then the dereference of this array didn't have a constant
> index
>  * (see the visit_continue_with_parent below), so we can't split
> @@ -350,6 +379,33 @@
> ir_array_splitting_visitor::visit_leave(ir_assignment *ir)
>  */
> ir_rvalue *lhs = ir->lhs;
>  
> +   /* "Unroll" any whole array assignments, creating assignments for
> +* each array element.  Then, do splitting on each new
> assignment.
> +*/
> +   if (lhs->type->is_array() && ir->whole_variable_written() &&
> +   get_splitting_entry(ir->whole_variable_written())) {
> +  void *mem_ctx = ralloc_parent(ir);
> +
> +  for (unsigned i = 0; i < lhs->type->length; i++) {
> + ir_rvalue *lhs_i =
> +new(mem_ctx) ir_dereference_array(ir->lhs-
> >clone(mem_ctx, NULL),
> +  new(mem_ctx)
> ir_constant(i));
> + ir_rvalue *rhs_i =
> +new(mem_ctx) ir_dereference_array(ir->rhs-
> >clone(mem_ctx, NULL),
> +  

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

2016-06-21 Thread Kenneth Graunke 
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 
---
 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();
+
+   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.
+*/
+   if (in_assignee && in_whole_array_copy)
+  return visit_continue;
+
/* If we made it to here without seeing an ir_dereference_array,
 * then the dereference of this array didn't have a constant index
 * (see the visit_continue_with_parent below), so we can't split
@@ -350,6 +379,33 @@ ir_array_splitting_visitor::visit_leave(ir_assignment *ir)
 */
ir_rvalue *lhs = ir->lhs;
 
+   /* "Unroll" any whole array assignments, creating assignments for
+* each array element.  Then, do splitting on each new assignment.
+*/
+   if (lhs->type->is_array() && ir->whole_variable_written() &&
+   get_splitting_entry(ir->whole_variable_written())) {
+  void *mem_ctx = ralloc_parent(ir);
+
+  for (unsigned i = 0; i < lhs->type->length; i++) {
+ ir_rvalue *lhs_i =
+new(mem_ctx) ir_dereference_array(ir->lhs->clone(mem_ctx, NULL),
+  new(mem_ctx) ir_constant(i));
+ ir_rvalue *rhs_i =
+new(mem_ctx) ir_dereference_array(ir->rhs->clone(mem_ctx, NULL),
+  new(mem_ctx) ir_constant(i));
+ ir_rvalue *condition_i =
+ir->condition ? ir->condition->clone(mem_ctx, NULL) : NULL;
+
+ ir_assignment *assign_i =
+new(mem_ctx) ir_assignment(lhs_i, rhs_i, condition_i);
+
+ ir->insert_before(assign_i);
+ assign_i->accept(this);
+  }
+  ir->remove();
+  return visit_continue;
+   }
+
handle_rvalue();
ir->lhs = lhs->as_dereference();
 
-- 
2.9.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev