On 05/06/2013 11:35 AM, Paul Berry wrote:
On 3 May 2013 16:07, Ian Romanick <i...@freedesktop.org
<mailto:i...@freedesktop.org>> wrote:
From: Ian Romanick <ian.d.roman...@intel.com
<mailto:ian.d.roman...@intel.com>>
Right now the lower_clip_distance_visitor lowers variable indexing into
gl_ClipDistance into variable indexing into both the array
gl_ClipDistanceMESA and the vectors of that array. For example,
gl_ClipDistance[i] = f;
becomes
gl_ClipDistanceMESA[i/4][i%4] = f;
Technically it becomes gl_ClipDistanceMESA[i>>2][i&3]. Granted it's
equivalent, but you might want to change your commit message to be more
in line with what the lowering pass actually does.
However, variable indexing into vectors using ir_dereference_array is
being removed. Instead, ir_expression with ir_triop_vector_insert will
be used. The above code will become
gl_ClipDistanceMESA[i/4] = vector_insert(gl_ClipDistanceMESA[i/4],
i % 4,
f);
In order to do this, an ir_rvalue_visitor will need to be used. This
commit is really just a refactor to get ready for that.
This commit is actually more than just a refactor. It also modifies how
the lowering pass treats statements of the form:
foo = gl_ClipDistance;
so that instead of being transformed to:
foo[0] = gl_ClipDistance[0][0];
foo[1] = gl_ClipDistance[0][1];
...
they are transformed to:
foo[0] = gl_ClipDistance[0].x;
foo[1] = gl_ClipDistance[0].y;
I don't really understand why this is necessary (since a later
optimization pass would make this transformation anyhow), and I believe
that it introduces a bug (see below). I'd prefer to see this
modification either dropped or split into its own patch, with an
explanation of why it is necessary.
The whole point of this entire series is getting rid of all instances of
array indexing of vectors. Previously in the series, everything that
could generate array indexing of a vector was modified to use the new
IR. The last patch in the series updates ir_validate to throw an error
if array indexing of a vector is encountered.
The lowering pass for gl_ClipDistance is the last thing in the compiler
that generates this sort of IR.
v2: Convert tabs to spaces. Suggested by Eric.
Signed-off-by: Ian Romanick <ian.d.roman...@intel.com
<mailto:ian.d.roman...@intel.com>>
Cc: Paul Berry <stereotype...@gmail.com
<mailto:stereotype...@gmail.com>>
---
src/glsl/lower_clip_distance.cpp | 136
+++++++++++++++++++++++++--------------
1 file changed, 86 insertions(+), 50 deletions(-)
diff --git a/src/glsl/lower_clip_distance.cpp
b/src/glsl/lower_clip_distance.cpp
index 643807d..19068fb 100644
--- a/src/glsl/lower_clip_distance.cpp
+++ b/src/glsl/lower_clip_distance.cpp
@@ -46,10 +46,10 @@
*/
#include "glsl_symbol_table.h"
-#include "ir_hierarchical_visitor.h"
+#include "ir_rvalue_visitor.h"
#include "ir.h"
-class lower_clip_distance_visitor : public ir_hierarchical_visitor {
+class lower_clip_distance_visitor : public ir_rvalue_visitor {
public:
lower_clip_distance_visitor()
: progress(false), old_clip_distance_var(NULL),
@@ -59,11 +59,12 @@ public:
virtual ir_visitor_status visit(ir_variable *);
void create_indices(ir_rvalue*, ir_rvalue *&, ir_rvalue *&);
- virtual ir_visitor_status visit_leave(ir_dereference_array *);
virtual ir_visitor_status visit_leave(ir_assignment *);
void visit_new_assignment(ir_assignment *ir);
virtual ir_visitor_status visit_leave(ir_call *);
+ virtual void handle_rvalue(ir_rvalue **rvalue);
+
bool progress;
/**
@@ -173,33 +174,35 @@
lower_clip_distance_visitor::create_indices(ir_rvalue *old_index,
}
-/**
- * Replace any expression that indexes into the gl_ClipDistance
array with an
- * expression that indexes into one of the vec4's in
gl_ClipDistanceMESA and
- * accesses the appropriate component.
- */
-ir_visitor_status
-lower_clip_distance_visitor::visit_leave(ir_dereference_array *ir)
+void
+lower_clip_distance_visitor::handle_rvalue(ir_rvalue **rv)
{
/* If the gl_ClipDistance var hasn't been declared yet, then
* there's no way this deref can refer to it.
*/
- if (!this->old_clip_distance_var)
- return visit_continue;
-
- ir_dereference_variable *old_var_ref =
ir->array->as_dereference_variable();
- if (old_var_ref && old_var_ref->var ==
this->old_clip_distance_var) {
- this->progress = true;
- ir_rvalue *array_index;
- ir_rvalue *swizzle_index;
- this->create_indices(ir->array_index, array_index,
swizzle_index);
- void *mem_ctx = ralloc_parent(ir);
- ir->array = new(mem_ctx) ir_dereference_array(
- this->new_clip_distance_var, array_index);
- ir->array_index = swizzle_index;
+ if (!this->old_clip_distance_var || *rv == NULL)
+ return;
+
+ ir_dereference_array *const array = (*rv)->as_dereference_array();
Can we rename this var to "array_deref"? It's confusing to see the
"array->array" below.
Okay.
+ if (array != NULL) {
+ /* Replace any expression that indexes into the
gl_ClipDistance array
+ * with an expression that indexes into one of the vec4's in
+ * gl_ClipDistanceMESA and accesses the appropriate component.
+ */
+ ir_dereference_variable *old_var_ref =
+ array->array->as_dereference_variable();
+ if (old_var_ref && old_var_ref->var ==
this->old_clip_distance_var) {
+ this->progress = true;
+ ir_rvalue *array_index;
+ ir_rvalue *swizzle_index;
+ this->create_indices(array->array_index, array_index,
swizzle_index);
+ void *mem_ctx = ralloc_parent(array);
+ array->array =
+ new(mem_ctx)
ir_dereference_array(this->new_clip_distance_var,
+ array_index);
+ array->array_index = swizzle_index;
+ }
}
-
- return visit_continue;
}
@@ -214,38 +217,71 @@
lower_clip_distance_visitor::visit_leave(ir_assignment *ir)
{
ir_dereference_variable *lhs_var =
ir->lhs->as_dereference_variable();
ir_dereference_variable *rhs_var =
ir->rhs->as_dereference_variable();
- if ((lhs_var && lhs_var->var == this->old_clip_distance_var)
- || (rhs_var && rhs_var->var == this->old_clip_distance_var)) {
- /* LHS or RHS of the assignment is the entire gl_ClipDistance
array.
- * Since we are reshaping gl_ClipDistance from an array of
floats to an
- * array of vec4's, this isn't going to work as a bulk assignment
- * anymore, so unroll it to element-by-element assignments
and lower
- * each of them.
- *
- * Note: to unroll into element-by-element assignments, we
need to make
- * clones of the LHS and RHS. This is only safe if the LHS
and RHS are
- * side-effect free. Fortunately, we know that they are,
because the
- * only kind of rvalue that can have side effects is an
ir_call, and
- * ir_calls only appear (a) as a statement on their own, or
(b) as the
- * RHS of an assignment that stores the result of the call in a
- * temporary variable.
- */
- void *ctx = ralloc_parent(ir);
+ void *ctx = ralloc_parent(ir);
+
+ /* LHS or RHS of the assignment is the entire gl_ClipDistance array.
+ * Since we are reshaping gl_ClipDistance from an array of
floats to an
+ * array of vec4's, this isn't going to work as a bulk assignment
+ * anymore, so unroll it to element-by-element assignments and lower
+ * each of them.
+ *
+ * Note: to unroll into element-by-element assignments, we need
to make
+ * clones of the LHS and RHS. This is only safe if the LHS and
RHS are
+ * side-effect free. Fortunately, we know that they are,
because the
+ * only kind of rvalue that can have side effects is an ir_call, and
+ * ir_calls only appear (a) as a statement on their own, or (b)
as the
+ * RHS of an assignment that stores the result of the call in a
+ * temporary variable.
+ */
The last two sentences of this comment are out of date, since a long
time ago we modified ir_call to be a statement rather than an
expression. If you're going to move this comment, please change the
last two sentences to something like "This is safe because expressions
are side-effect free".
Okay.
+ if (lhs_var && lhs_var->var == this->old_clip_distance_var) {
int array_size =
this->old_clip_distance_var->type->array_size();
for (int i = 0; i < array_size; ++i) {
- ir_dereference_array *new_lhs = new(ctx) ir_dereference_array(
- ir->lhs->clone(ctx, NULL), new(ctx) ir_constant(i));
- new_lhs->accept(this);
+ ir_dereference_variable *const new_deref =
+ new(ctx) ir_dereference_variable(new_clip_distance_var);
+
+ ir_dereference_array *const new_lhs =
+ new(ctx) ir_dereference_array(new_deref,
+ new(ctx) ir_constant(i / 4));
+
ir_dereference_array *new_rhs = new(ctx)
ir_dereference_array(
ir->rhs->clone(ctx, NULL), new(ctx) ir_constant(i));
- new_rhs->accept(this);
- this->base_ir->insert_before(
- new(ctx) ir_assignment(new_lhs, new_rhs));
+
+ const int mask = 1 << (i % 4);
+
+ ir_assignment *const assign =
+ new(ctx) ir_assignment(new_lhs, new_rhs, NULL, mask);
+
+ this->base_ir->insert_before(assign);
}
ir->remove();
+
+ return visit_continue;
+ } else if (rhs_var && rhs_var->var == this->old_clip_distance_var) {
+ int array_size = this->old_clip_distance_var->type->array_size();
+ for (int i = 0; i < array_size; ++i) {
+ ir_dereference_array *new_lhs = new(ctx) ir_dereference_array(
+ ir->lhs->clone(ctx, NULL), new(ctx) ir_constant(i));
+
+ ir_dereference_variable *const new_deref =
+ new(ctx) ir_dereference_variable(new_clip_distance_var);
+
+ ir_rvalue *const new_rhs =
+ new(ctx) ir_swizzle(new(ctx)
ir_dereference_array(new_deref,
+
new(ctx) ir_constant(i / 4)),
+ i % 4, 0, 0, 0, 1);
+
+ ir_assignment *const assign =
+ new(ctx) ir_assignment(new_lhs, new_rhs);
+
+ this->base_ir->insert_before(assign);
+ }
+ ir->remove();
+
+ return visit_continue;
}
As I alluded to above, it's not clear to me why splitting this code into
two separate cases (one for LHS, one for RHS) is necessary/beneficial.
Because there are two kinds of IR that replace array indexing of
vectors: ir_binop_vector_extract and ir_triop_vector_insert. The next
patch makes this code use those.
Also, I suspect that we now have a bug if the shader contains the
(admittedly silly) statement:
gl_ClipDistance = gl_ClipDistance;
I don't think so. Since this is an array assignment, it will already be
broken down into a sequence of
gl_ClipDistance[0] = gl_ClipDistance[0];
...
gl_ClipDistance[7] = gl_ClipDistance[7];
Right? Each of those should get handled correctly. I was also pretty
sure that you added a test case for this when you originally wrote this
lowering pass. Am I misremembering?
- return visit_continue;
+ handle_rvalue((ir_rvalue **)&ir->lhs);
+ return rvalue_visit(ir);
I could have really used a comment explaining why the call to
handle_rvalue() is necessary here. Something like:
"rvalue_visit(ir_assignment*) only visits the RHS of the assignment, but
we need references to gl_ClipDistance in the LHS to be lowered also."
Okay.
}
@@ -330,7 +366,7 @@ lower_clip_distance_visitor::visit_leave(ir_call
*ir)
}
}
- return visit_continue;
+ return rvalue_visit(ir);
}
--
1.8.1.4
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
