<div dir="ltr">On 3 May 2013 16:07, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>><br>
<br>
Right now the lower_clip_distance_visitor lowers variable indexing into<br>
gl_ClipDistance into variable indexing into both the array<br>
gl_ClipDistanceMESA and the vectors of that array. For example,<br>
<br>
gl_ClipDistance[i] = f;<br>
<br>
becomes<br>
<br>
gl_ClipDistanceMESA[i/4][i%4] = f;<br></blockquote><div><br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
However, variable indexing into vectors using ir_dereference_array is<br>
being removed. Instead, ir_expression with ir_triop_vector_insert will<br>
be used. The above code will become<br>
<br>
gl_ClipDistanceMESA[i/4] = vector_insert(gl_ClipDistanceMESA[i/4],<br>
i % 4,<br>
f);<br>
<br>
In order to do this, an ir_rvalue_visitor will need to be used. This<br>
commit is really just a refactor to get ready for that.<br></blockquote><div><br></div><div>This commit is actually more than just a refactor. It also modifies how the lowering pass treats statements of the form:<br><br>
</div><div> foo = gl_ClipDistance;<br><br></div><div>so that instead of being transformed to:<br><br></div><div> foo[0] = gl_ClipDistance[0][0];<br></div><div> foo[1] = gl_ClipDistance[0][1];<br> ...<br><br></div>
<div>they are transformed to:<br><br></div><div> foo[0] = gl_ClipDistance[0].x;<br></div><div> foo[1] = gl_ClipDistance[0].y;<br><br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
v2: Convert tabs to spaces. Suggested by Eric.<br>
<br>
Signed-off-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>><br>
Cc: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br>
---<br>
src/glsl/lower_clip_distance.cpp | 136 +++++++++++++++++++++++++--------------<br>
1 file changed, 86 insertions(+), 50 deletions(-)<br>
<br>
diff --git a/src/glsl/lower_clip_distance.cpp b/src/glsl/lower_clip_distance.cpp<br>
index 643807d..19068fb 100644<br>
--- a/src/glsl/lower_clip_distance.cpp<br>
+++ b/src/glsl/lower_clip_distance.cpp<br>
@@ -46,10 +46,10 @@<br>
*/<br>
<br>
#include "glsl_symbol_table.h"<br>
-#include "ir_hierarchical_visitor.h"<br>
+#include "ir_rvalue_visitor.h"<br>
#include "ir.h"<br>
<br>
-class lower_clip_distance_visitor : public ir_hierarchical_visitor {<br>
+class lower_clip_distance_visitor : public ir_rvalue_visitor {<br>
public:<br>
lower_clip_distance_visitor()<br>
: progress(false), old_clip_distance_var(NULL),<br>
@@ -59,11 +59,12 @@ public:<br>
<br>
virtual ir_visitor_status visit(ir_variable *);<br>
void create_indices(ir_rvalue*, ir_rvalue *&, ir_rvalue *&);<br>
- virtual ir_visitor_status visit_leave(ir_dereference_array *);<br>
virtual ir_visitor_status visit_leave(ir_assignment *);<br>
void visit_new_assignment(ir_assignment *ir);<br>
virtual ir_visitor_status visit_leave(ir_call *);<br>
<br>
+ virtual void handle_rvalue(ir_rvalue **rvalue);<br>
+<br>
bool progress;<br>
<br>
/**<br>
@@ -173,33 +174,35 @@ lower_clip_distance_visitor::create_indices(ir_rvalue *old_index,<br>
}<br>
<br>
<br>
-/**<br>
- * Replace any expression that indexes into the gl_ClipDistance array with an<br>
- * expression that indexes into one of the vec4's in gl_ClipDistanceMESA and<br>
- * accesses the appropriate component.<br>
- */ <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
-ir_visitor_status<br>
-lower_clip_distance_visitor::visit_leave(ir_dereference_array *ir)<br>
+void<br>
+lower_clip_distance_visitor::handle_rvalue(ir_rvalue **rv)<br>
{<br>
/* If the gl_ClipDistance var hasn't been declared yet, then<br>
* there's no way this deref can refer to it.<br>
*/<br>
- if (!this->old_clip_distance_var)<br>
- return visit_continue;<br>
-<br>
- ir_dereference_variable *old_var_ref = ir->array->as_dereference_variable();<br>
- if (old_var_ref && old_var_ref->var == this->old_clip_distance_var) {<br>
- this->progress = true;<br>
- ir_rvalue *array_index;<br>
- ir_rvalue *swizzle_index;<br>
- this->create_indices(ir->array_index, array_index, swizzle_index);<br>
- void *mem_ctx = ralloc_parent(ir);<br>
- ir->array = new(mem_ctx) ir_dereference_array(<br>
- this->new_clip_distance_var, array_index);<br>
- ir->array_index = swizzle_index;<br>
+ if (!this->old_clip_distance_var || *rv == NULL)<br>
+ return; <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ ir_dereference_array *const array = (*rv)->as_dereference_array();<br></blockquote><div><br></div><div>Can we rename this var to "array_deref"? It's confusing to see the "array->array" below.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (array != NULL) {<br>
+ /* Replace any expression that indexes into the gl_ClipDistance array<br>
+ * with an expression that indexes into one of the vec4's in<br>
+ * gl_ClipDistanceMESA and accesses the appropriate component.<br>
+ */<br>
+ ir_dereference_variable *old_var_ref =<br>
+ array->array->as_dereference_variable();<br>
+ if (old_var_ref && old_var_ref->var == this->old_clip_distance_var) {<br>
+ this->progress = true;<br>
+ ir_rvalue *array_index;<br>
+ ir_rvalue *swizzle_index;<br>
+ this->create_indices(array->array_index, array_index, swizzle_index);<br>
+ void *mem_ctx = ralloc_parent(array);<br>
+ array->array =<br>
+ new(mem_ctx) ir_dereference_array(this->new_clip_distance_var,<br>
+ array_index);<br>
+ array->array_index = swizzle_index;<br>
+ }<br>
}<br>
-<br>
- return visit_continue;<br>
}<br>
<br>
<br>
@@ -214,38 +217,71 @@ lower_clip_distance_visitor::visit_leave(ir_assignment *ir)<br>
{<br>
ir_dereference_variable *lhs_var = ir->lhs->as_dereference_variable();<br>
ir_dereference_variable *rhs_var = ir->rhs->as_dereference_variable();<br>
- if ((lhs_var && lhs_var->var == this->old_clip_distance_var)<br>
- || (rhs_var && rhs_var->var == this->old_clip_distance_var)) {<br>
- /* LHS or RHS of the assignment is the entire gl_ClipDistance array.<br>
- * Since we are reshaping gl_ClipDistance from an array of floats to an<br>
- * array of vec4's, this isn't going to work as a bulk assignment<br>
- * anymore, so unroll it to element-by-element assignments and lower<br>
- * each of them.<br>
- *<br>
- * Note: to unroll into element-by-element assignments, we need to make<br>
- * clones of the LHS and RHS. This is only safe if the LHS and RHS are<br>
- * side-effect free. Fortunately, we know that they are, because the<br>
- * only kind of rvalue that can have side effects is an ir_call, and<br>
- * ir_calls only appear (a) as a statement on their own, or (b) as the<br>
- * RHS of an assignment that stores the result of the call in a<br>
- * temporary variable.<br>
- */<br>
- void *ctx = ralloc_parent(ir);<br>
+ void *ctx = ralloc_parent(ir);<br>
+<br>
+ /* LHS or RHS of the assignment is the entire gl_ClipDistance array.<br>
+ * Since we are reshaping gl_ClipDistance from an array of floats to an<br>
+ * array of vec4's, this isn't going to work as a bulk assignment<br>
+ * anymore, so unroll it to element-by-element assignments and lower<br>
+ * each of them.<br>
+ *<br>
+ * Note: to unroll into element-by-element assignments, we need to make<br>
+ * clones of the LHS and RHS. This is only safe if the LHS and RHS are<br>
+ * side-effect free. Fortunately, we know that they are, because the<br>
+ * only kind of rvalue that can have side effects is an ir_call, and<br>
+ * ir_calls only appear (a) as a statement on their own, or (b) as the<br>
+ * RHS of an assignment that stores the result of the call in a<br>
+ * temporary variable.<br>
+ */<br></blockquote><div><br></div><div>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".<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (lhs_var && lhs_var->var == this->old_clip_distance_var) {<br>
int array_size = this->old_clip_distance_var->type->array_size();<br>
for (int i = 0; i < array_size; ++i) {<br>
- ir_dereference_array *new_lhs = new(ctx) ir_dereference_array(<br>
- ir->lhs->clone(ctx, NULL), new(ctx) ir_constant(i));<br>
- new_lhs->accept(this);<br>
+ ir_dereference_variable *const new_deref =<br>
+ new(ctx) ir_dereference_variable(new_clip_distance_var);<br>
+<br>
+ ir_dereference_array *const new_lhs =<br>
+ new(ctx) ir_dereference_array(new_deref,<br>
+ new(ctx) ir_constant(i / 4));<br>
+<br>
ir_dereference_array *new_rhs = new(ctx) ir_dereference_array(<br>
ir->rhs->clone(ctx, NULL), new(ctx) ir_constant(i));<br>
- new_rhs->accept(this);<br>
- this->base_ir->insert_before(<br>
- new(ctx) ir_assignment(new_lhs, new_rhs));<br>
+<br>
+ const int mask = 1 << (i % 4);<br>
+<br>
+ ir_assignment *const assign =<br>
+ new(ctx) ir_assignment(new_lhs, new_rhs, NULL, mask);<br>
+<br>
+ this->base_ir->insert_before(assign);<br>
}<br>
ir->remove();<br>
+<br>
+ return visit_continue;<br>
+ } else if (rhs_var && rhs_var->var == this->old_clip_distance_var) {<br>
+ int array_size = this->old_clip_distance_var->type->array_size();<br>
+ for (int i = 0; i < array_size; ++i) {<br>
+ ir_dereference_array *new_lhs = new(ctx) ir_dereference_array(<br>
+ ir->lhs->clone(ctx, NULL), new(ctx) ir_constant(i));<br>
+<br>
+ ir_dereference_variable *const new_deref =<br>
+ new(ctx) ir_dereference_variable(new_clip_distance_var);<br>
+<br>
+ ir_rvalue *const new_rhs =<br>
+ new(ctx) ir_swizzle(new(ctx) ir_dereference_array(new_deref,<br>
+ new(ctx) ir_constant(i / 4)),<br>
+ i % 4, 0, 0, 0, 1);<br>
+<br>
+ ir_assignment *const assign =<br>
+ new(ctx) ir_assignment(new_lhs, new_rhs);<br>
+<br>
+ this->base_ir->insert_before(assign);<br>
+ }<br>
+ ir->remove();<br>
+<br>
+ return visit_continue;<br>
}<br></blockquote><div><br></div><div>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. Also, I suspect that we now have a bug if the shader contains the (admittedly silly) statement:<br>
<br></div><div> gl_ClipDistance = gl_ClipDistance;<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- return visit_continue;<br>
+ handle_rvalue((ir_rvalue **)&ir->lhs);<br>
+ return rvalue_visit(ir);<br></blockquote><div><br></div><div>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."<br>
</div><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
<br>
<br>
@@ -330,7 +366,7 @@ lower_clip_distance_visitor::visit_leave(ir_call *ir)<br>
}<br>
}<br>
<br>
- return visit_continue;<br>
+ return rvalue_visit(ir);<br>
}<br>
<span class="HOEnZb"><font color="#888888"><br>
<br>
--<br>
1.8.1.4<br>
<br>
</font></span></blockquote></div><br></div></div>