On Mon, 18 Jul 2011 14:15:22 -0700, "Ian Romanick" <i...@freedesktop.org> wrote: > From: Ian Romanick <ian.d.roman...@intel.com> > > The previous implementation could easily get tricked if the LHS of an > assignment included a non-constant index that was "inside" another > dereference. For example: > > mat4 m[2]; > m[0][i] = vec4(0.0); > > Due to the way it tracked whether the array was being assigned, it > would think that the non-constant index was in an r-value. The new > code fixes that by tracking l-values and r-values differently. The > index is also replaced by cloning the IR and replacing the index > variable instead of the odd way it was done before. > > Fixes i965 piglit fs-temp-array-mat[234]-index-wr and > vs-varying-array-mat[234]-index-wr.
I think I see one little simplification, and other than that I think
I've made sense of it.
> struct assignment_generator
> {
> ir_instruction* base_ir;
> - ir_rvalue* array;
> + ir_rvalue *rvalue;
I think this rvalue could be an ir_dereference *
> + ir_variable *old_index;
> bool is_write;
> unsigned int write_mask;
> ir_variable* var;
> @@ -61,14 +136,25 @@ struct assignment_generator
> * underlying variable.
> */
> void *mem_ctx = ralloc_parent(base_ir);
> - ir_dereference *element =
> - new(mem_ctx) ir_dereference_array(this->array->clone(mem_ctx, NULL),
> - new(mem_ctx) ir_constant(i));
> - ir_rvalue *variable = new(mem_ctx) ir_dereference_variable(this->var);
>
> + /* Clone the old r-value in its entirety. Then replace any occurances
> of
> + * the old variable index with the new constant index.
> + */
> + ir_rvalue *element = this->rvalue->clone(mem_ctx, NULL);
Making this an ir_dereference *
> + ir_constant *const index = new(mem_ctx) ir_constant(i);
> + deref_replacer r(this->old_index, index);
> + element->accept(&r);
> + assert(r.progress);
> +
> + /* Generate a conditional assignment to (or from) the constant indexed
> + * array dereference.
> + */
> + ir_rvalue *variable = new(mem_ctx) ir_dereference_variable(this->var);
> ir_assignment *assignment;
> if (is_write) {
> - assignment = new(mem_ctx) ir_assignment(element, variable, condition,
> + ir_dereference *d = element->as_dereference();
> + assert(d != NULL);
> + assignment = new(mem_ctx) ir_assignment(d, variable, condition,
> write_mask);
And avoiding this cast/assert...
> ir_variable *convert_dereference_array(ir_dereference_array *orig_deref,
> - ir_assignment* orig_assign)
> + ir_assignment* orig_assign,
> + ir_rvalue *orig_base)
> {
> assert(is_array_or_matrix(orig_deref->array));
>
> @@ -320,9 +407,12 @@ public:
> new(mem_ctx) ir_assignment(lhs, orig_deref->array_index, NULL);
> base_ir->insert_before(assign);
>
> + orig_deref->array_index = lhs->clone(mem_ctx, NULL);
> +
> assignment_generator ag;
> - ag.array = orig_deref->array;
> + ag.rvalue = orig_base;
> ag.base_ir = base_ir;
> + ag.old_index = index;
> ag.var = var;
> if (orig_assign) {
> ag.is_write = true;
> @@ -342,12 +432,16 @@ public:
>
> virtual void handle_rvalue(ir_rvalue **pir)
> {
> + if (this->in_assignee)
> + return;
> +
> if (!*pir)
> return;
>
> ir_dereference_array* orig_deref = (*pir)->as_dereference_array();
> if (needs_lowering(orig_deref)) {
> - ir_variable* var = convert_dereference_array(orig_deref, 0);
> + ir_variable *var =
> + convert_dereference_array(orig_deref, NULL, orig_deref);
> assert(var);
> *pir = new(ralloc_parent(base_ir)) ir_dereference_variable(var);
> this->progress = true;
> @@ -359,10 +453,11 @@ public:
> {
> ir_rvalue_visitor::visit_leave(ir);
>
> - ir_dereference_array *orig_deref = ir->lhs->as_dereference_array();
> + find_variable_index f;
> + ir->lhs->accept(&f);
>
> - if (needs_lowering(orig_deref)) {
> - convert_dereference_array(orig_deref, ir);
> + if ((f.deref != NULL) && storage_type_needs_lowering(f.deref)) {
> + convert_dereference_array(f.deref, ir, ir->lhs);
> ir->remove();
> this->progress = true;
> }
Because the orig_base of both convert_dereference_array() calls is a
deref.
I think it's nice to note that this is a dereference because it means
that clone()ing should be safer than, say, cloning some expression tree.
pgpBpnJtPDJim.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
