[Mesa-dev] [PATCH 09/12] glsl: Convert lower_clip_distance_visitor to be an ir_rvalue_visitor
Paul Berry
stereotype441 at gmail.com
Mon May 6 11:35:06 PDT 2013
On 3 May 2013 16:07, Ian Romanick <idr at freedesktop.org> wrote:
> From: Ian Romanick <ian.d.romanick at 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.
>
> v2: Convert tabs to spaces. Suggested by Eric.
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Cc: Paul Berry <stereotype441 at 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.
> + 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".
> + 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.
Also, I suspect that we now have a bug if the shader contains the
(admittedly silly) statement:
gl_ClipDistance = gl_ClipDistance;
>
> - 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."
}
>
>
> @@ -330,7 +366,7 @@ lower_clip_distance_visitor::visit_leave(ir_call *ir)
> }
> }
>
> - return visit_continue;
> + return rvalue_visit(ir);
> }
>
>
> --
> 1.8.1.4
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130506/56b88854/attachment-0001.html>
More information about the mesa-dev
mailing list