[Mesa-dev] [PATCH 3/8] glsl: Rework lowering of non-constant array indexing

[Eric Anholt]

On Mon, 18 Jul 2011 14:15:22 -0700, "Ian Romanick" <idr at freedesktop.org> wrote:
> From: Ian Romanick <ian.d.romanick at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110720/18b79779/attachment-0001.pgp>