[Mesa-dev] [PATCH 09/12] glsl: Convert lower_clip_distance_visitor to be an ir_rvalue_visitor
Ian Romanick
idr at freedesktop.org
Mon May 6 12:47:58 PDT 2013
On 05/06/2013 11:35 AM, Paul Berry wrote:
> On 3 May 2013 16:07, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
> From: Ian Romanick <ian.d.romanick at intel.com
> <mailto: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.
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.romanick at intel.com
> <mailto:ian.d.romanick at intel.com>>
> Cc: Paul Berry <stereotype441 at gmail.com
> <mailto: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.
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
>
>
More information about the mesa-dev
mailing list