[Mesa-dev] [PATCH 4/5] nir: Allocate dereferences out of their parent instruction or deref.

Jason Ekstrand jason at jlekstrand.net
Tue Apr 7 07:58:46 PDT 2015


Other than my nitpicking below this looks great! Thanks for working on this!
On Apr 7, 2015 2:32 AM, "Kenneth Graunke" <kenneth at whitecape.org> wrote:
>
> Jason pointed out that variable dereferences in NIR are really part of
> their parent instruction, and should have the same lifetime.
>
> Unlike in GLSL IR, they're not used very often - just for intrinsic
> variables, call parameters & return, and indirect samplers for
> texturing.  Also, nir_deref_var is the top-level concept, and
> nir_deref_array/nir_deref_record are child nodes.
>
> This patch attempts to allocate nir_deref_vars out of their parent
> instruction, and any sub-dereferences out of their parent deref.
> It enforces these restrictions in the validator as well.
>
> This means that freeing an instruction should free its associated
> dereference chain as well.  The memory sweeper pass can also happily
> ignore them.
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/glsl/nir/glsl_to_nir.cpp        | 47
++++++++++++++++++++-----------------
>  src/glsl/nir/nir.c                  |  6 ++---
>  src/glsl/nir/nir_lower_var_copies.c |  8 +++----
>  src/glsl/nir/nir_split_var_copies.c |  4 ++--
>  src/glsl/nir/nir_validate.c         | 13 ++++++----
>  src/mesa/program/prog_to_nir.c      |  9 ++++---
>  6 files changed, 45 insertions(+), 42 deletions(-)
>
> This is still a lot of churn, but surprisingly about even on LOC.
> With the validator code in place, I suspect we can get this right
> going forward without too much trouble.
>
> diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
> index 80c5b3a..f61a47a 100644
> --- a/src/glsl/nir/glsl_to_nir.cpp
> +++ b/src/glsl/nir/glsl_to_nir.cpp
> @@ -88,6 +88,8 @@ private:
>     exec_list *cf_node_list;
>     nir_instr *result; /* result of the expression tree last visited */
>
> +   nir_deref_var *make_deref(void *mem_ctx, ir_instruction *ir);
> +
>     /* the head of the dereference chain we're creating */
>     nir_deref_var *deref_head;
>     /* the tail of the dereference chain we're creating */
> @@ -156,6 +158,14 @@ nir_visitor::~nir_visitor()
>     _mesa_hash_table_destroy(this->overload_table, NULL);
>  }
>
> +nir_deref_var *
> +nir_visitor::make_deref(void *mem_ctx, ir_instruction *ir)

I'm not a huge fan of the name. Maybe evaluate_deref to match
evaluate_rvalue or perhaps build_deref?  In any case, it doesn't really
matter so I won't quibble.

It should, however, take a nir_instr instead of a void as its memory
context.  That makes it a bit more explicit.

> +{
> +   ir->accept(this);
> +   ralloc_steal(mem_ctx, this->deref_head);
> +   return this->deref_head;
> +}
> +
>  static nir_constant *
>  constant_copy(ir_constant *ir, void *mem_ctx)
>  {
> @@ -582,13 +592,11 @@ void
>  nir_visitor::visit(ir_return *ir)
>  {
>     if (ir->value != NULL) {
> -      ir->value->accept(this);
>        nir_intrinsic_instr *copy =
>           nir_intrinsic_instr_create(this->shader,
nir_intrinsic_copy_var);
>
> -      copy->variables[0] = nir_deref_var_create(this->shader,
> -                                                this->impl->return_var);
> -      copy->variables[1] = this->deref_head;
> +      copy->variables[0] = nir_deref_var_create(copy,
this->impl->return_var);
> +      copy->variables[1] = make_deref(copy, ir->value);
>     }
>
>     nir_jump_instr *instr = nir_jump_instr_create(this->shader,
nir_jump_return);
> @@ -613,8 +621,7 @@ nir_visitor::visit(ir_call *ir)
>        nir_intrinsic_instr *instr = nir_intrinsic_instr_create(shader,
op);
>        ir_dereference *param =
>           (ir_dereference *) ir->actual_parameters.get_head();
> -      param->accept(this);
> -      instr->variables[0] = this->deref_head;
> +      instr->variables[0] = make_deref(instr, param);
>        nir_ssa_dest_init(&instr->instr, &instr->dest, 1, NULL);
>
>        nir_instr_insert_after_cf_list(this->cf_node_list, &instr->instr);
> @@ -623,8 +630,7 @@ nir_visitor::visit(ir_call *ir)
>           nir_intrinsic_instr_create(shader, nir_intrinsic_store_var);
>        store_instr->num_components = 1;
>
> -      ir->return_deref->accept(this);
> -      store_instr->variables[0] = this->deref_head;
> +      store_instr->variables[0] = make_deref(store_instr,
ir->return_deref);
>        store_instr->src[0].is_ssa = true;
>        store_instr->src[0].ssa = &instr->dest.ssa;
>
> @@ -642,13 +648,11 @@ nir_visitor::visit(ir_call *ir)
>
>     unsigned i = 0;
>     foreach_in_list(ir_dereference, param, &ir->actual_parameters) {
> -      param->accept(this);
> -      instr->params[i] = this->deref_head;
> +      instr->params[i] = make_deref(instr, param);
>        i++;
>     }
>
> -   ir->return_deref->accept(this);
> -   instr->return_deref = this->deref_head;
> +   instr->return_deref = make_deref(instr, ir->return_deref);
>     nir_instr_insert_after_cf_list(this->cf_node_list, &instr->instr);
>  }
>
> @@ -663,12 +667,8 @@ nir_visitor::visit(ir_assignment *ir)
>        nir_intrinsic_instr *copy =
>           nir_intrinsic_instr_create(this->shader,
nir_intrinsic_copy_var);
>
> -      ir->lhs->accept(this);
> -      copy->variables[0] = this->deref_head;
> -
> -      ir->rhs->accept(this);
> -      copy->variables[1] = this->deref_head;
> -
> +      copy->variables[0] = make_deref(copy, ir->lhs);
> +      copy->variables[1] = make_deref(copy, ir->rhs);
>
>        if (ir->condition) {
>           nir_if *if_stmt = nir_if_create(this->shader);
> @@ -700,6 +700,7 @@ nir_visitor::visit(ir_assignment *ir)
>        load->num_components = ir->lhs->type->vector_elements;
>        nir_ssa_dest_init(&load->instr, &load->dest, num_components, NULL);
>        load->variables[0] = lhs_deref;
> +      ralloc_steal(load, load->variables[0]);
>        nir_instr_insert_after_cf_list(this->cf_node_list, &load->instr);
>
>        nir_op vec_op;
> @@ -741,7 +742,7 @@ nir_visitor::visit(ir_assignment *ir)
>     nir_intrinsic_instr *store =
>        nir_intrinsic_instr_create(this->shader, nir_intrinsic_store_var);
>     store->num_components = ir->lhs->type->vector_elements;
> -   nir_deref *store_deref = nir_copy_deref(this->shader,
&lhs_deref->deref);
> +   nir_deref *store_deref = nir_copy_deref(store, &lhs_deref->deref);
>     store->variables[0] = nir_deref_as_var(store_deref);
>     store->src[0] = src;
>
> @@ -816,6 +817,7 @@ nir_visitor::evaluate_rvalue(ir_rvalue* ir)
>           nir_intrinsic_instr_create(this->shader,
nir_intrinsic_load_var);
>        load_instr->num_components = ir->type->vector_elements;
>        load_instr->variables[0] = this->deref_head;
> +      ralloc_steal(load_instr, load_instr->variables[0]);
>        add_instr(&load_instr->instr, ir->type->vector_elements);
>     }
>
> @@ -959,6 +961,7 @@ nir_visitor::visit(ir_expression *ir)
>        nir_intrinsic_instr *intrin = nir_intrinsic_instr_create(shader,
op);
>        intrin->num_components = deref->type->vector_elements;
>        intrin->variables[0] = this->deref_head;
> +      ralloc_steal(intrin, intrin->variables[0]);
>
>        if (intrin->intrinsic == nir_intrinsic_interp_var_at_offset ||
>            intrin->intrinsic == nir_intrinsic_interp_var_at_sample)
> @@ -1630,8 +1633,7 @@ nir_visitor::visit(ir_texture *ir)
>        unreachable("not reached");
>     }
>
> -   ir->sampler->accept(this);
> -   instr->sampler = this->deref_head;
> +   instr->sampler = make_deref(instr, ir->sampler);
>
>     unsigned src_number = 0;
>
> @@ -1756,7 +1758,7 @@ nir_visitor::visit(ir_dereference_record *ir)
>     int field_index = this->deref_tail->type->field_index(ir->field);
>     assert(field_index >= 0);
>
> -   nir_deref_struct *deref = nir_deref_struct_create(this->shader,
field_index);
> +   nir_deref_struct *deref = nir_deref_struct_create(this->deref_tail,
field_index);
>     deref->deref.type = ir->type;
>     this->deref_tail->child = &deref->deref;
>     this->deref_tail = &deref->deref;
> @@ -1780,5 +1782,6 @@ nir_visitor::visit(ir_dereference_array *ir)
>     ir->array->accept(this);
>
>     this->deref_tail->child = &deref->deref;
> +   ralloc_steal(this->deref_tail, deref);
>     this->deref_tail = &deref->deref;
>  }
> diff --git a/src/glsl/nir/nir.c b/src/glsl/nir/nir.c
> index 85ff0f4..1c6b603 100644
> --- a/src/glsl/nir/nir.c
> +++ b/src/glsl/nir/nir.c
> @@ -543,7 +543,7 @@ copy_deref_var(void *mem_ctx, nir_deref_var *deref)
>     nir_deref_var *ret = nir_deref_var_create(mem_ctx, deref->var);
>     ret->deref.type = deref->deref.type;
>     if (deref->deref.child)
> -      ret->deref.child = nir_copy_deref(mem_ctx, deref->deref.child);
> +      ret->deref.child = nir_copy_deref(ret, deref->deref.child);
>     return ret;
>  }
>
> @@ -558,7 +558,7 @@ copy_deref_array(void *mem_ctx, nir_deref_array
*deref)
>     }
>     ret->deref.type = deref->deref.type;
>     if (deref->deref.child)
> -      ret->deref.child = nir_copy_deref(mem_ctx, deref->deref.child);
> +      ret->deref.child = nir_copy_deref(ret, deref->deref.child);
>     return ret;
>  }
>
> @@ -568,7 +568,7 @@ copy_deref_struct(void *mem_ctx, nir_deref_struct
*deref)
>     nir_deref_struct *ret = nir_deref_struct_create(mem_ctx,
deref->index);
>     ret->deref.type = deref->deref.type;
>     if (deref->deref.child)
> -      ret->deref.child = nir_copy_deref(mem_ctx, deref->deref.child);
> +      ret->deref.child = nir_copy_deref(ret, deref->deref.child);
>     return ret;
>  }

I'd really like to make nir_copy_deref operate on deref_var's and take a
nir_instr as well but that can be another patch later.

> diff --git a/src/glsl/nir/nir_lower_var_copies.c
b/src/glsl/nir/nir_lower_var_copies.c
> index 85ebb28..58389a7 100644
> --- a/src/glsl/nir/nir_lower_var_copies.c
> +++ b/src/glsl/nir/nir_lower_var_copies.c
> @@ -148,13 +148,10 @@ emit_copy_load_store(nir_intrinsic_instr
*copy_instr,
>
>        unsigned num_components = glsl_get_vector_elements(src_tail->type);
>
> -      nir_deref *src_deref = nir_copy_deref(mem_ctx, &src_head->deref);
> -      nir_deref *dest_deref = nir_copy_deref(mem_ctx, &dest_head->deref);
> -
>        nir_intrinsic_instr *load =
>           nir_intrinsic_instr_create(mem_ctx, nir_intrinsic_load_var);
>        load->num_components = num_components;
> -      load->variables[0] = nir_deref_as_var(src_deref);
> +      load->variables[0] = nir_deref_as_var(nir_copy_deref(load,
&src_head->deref));
>        nir_ssa_dest_init(&load->instr, &load->dest, num_components, NULL);
>
>        nir_instr_insert_before(&copy_instr->instr, &load->instr);
> @@ -162,7 +159,8 @@ emit_copy_load_store(nir_intrinsic_instr *copy_instr,
>        nir_intrinsic_instr *store =
>           nir_intrinsic_instr_create(mem_ctx, nir_intrinsic_store_var);
>        store->num_components = num_components;
> -      store->variables[0] = nir_deref_as_var(dest_deref);
> +      store->variables[0] = nir_deref_as_var(nir_copy_deref(store,
&dest_head->deref));
> +
>        store->src[0].is_ssa = true;
>        store->src[0].ssa = &load->dest.ssa;
>
> diff --git a/src/glsl/nir/nir_split_var_copies.c
b/src/glsl/nir/nir_split_var_copies.c
> index 4d663b5..fc72c07 100644
> --- a/src/glsl/nir/nir_split_var_copies.c
> +++ b/src/glsl/nir/nir_split_var_copies.c
> @@ -188,8 +188,8 @@ split_var_copy_instr(nir_intrinsic_instr *old_copy,
>            * belongs to the copy instruction and b) the deref chains may
>            * have some of the same links due to the way we constructed
them
>            */
> -         nir_deref *src = nir_copy_deref(state->mem_ctx, src_head);
> -         nir_deref *dest = nir_copy_deref(state->mem_ctx, dest_head);
> +         nir_deref *src = nir_copy_deref(new_copy, src_head);
> +         nir_deref *dest = nir_copy_deref(new_copy, dest_head);
>
>           new_copy->variables[0] = nir_deref_as_var(dest);
>           new_copy->variables[1] = nir_deref_as_var(src);
> diff --git a/src/glsl/nir/nir_validate.c b/src/glsl/nir/nir_validate.c
> index e8c9d7b..a7aa798 100644
> --- a/src/glsl/nir/nir_validate.c
> +++ b/src/glsl/nir/nir_validate.c
> @@ -295,6 +295,8 @@ validate_alu_instr(nir_alu_instr *instr,
validate_state *state)
>  static void
>  validate_deref_chain(nir_deref *deref, validate_state *state)
>  {
> +   assert(deref->child == NULL || ralloc_parent(deref->child) == deref);
> +
>     nir_deref *parent = NULL;
>     while (deref != NULL) {
>        switch (deref->deref_type) {
> @@ -336,9 +338,10 @@ validate_var_use(nir_variable *var, validate_state
*state)
>  }
>
>  static void
> -validate_deref_var(nir_deref_var *deref, validate_state *state)
> +validate_deref_var(void *parent_mem_ctx, nir_deref_var *deref,
validate_state *state)
>  {
>     assert(deref != NULL);
> +   assert(ralloc_parent(deref) == parent_mem_ctx);

Thanks for validating this!

>     assert(deref->deref.type == deref->var->type);
>
>     validate_var_use(deref->var, state);
> @@ -386,7 +389,7 @@ validate_intrinsic_instr(nir_intrinsic_instr *instr,
validate_state *state)
>
>     unsigned num_vars =
nir_intrinsic_infos[instr->intrinsic].num_variables;
>     for (unsigned i = 0; i < num_vars; i++) {
> -      validate_deref_var(instr->variables[i], state);
> +      validate_deref_var(instr, instr->variables[i], state);
>     }
>
>     switch (instr->intrinsic) {
> @@ -423,7 +426,7 @@ validate_tex_instr(nir_tex_instr *instr,
validate_state *state)
>     }
>
>     if (instr->sampler != NULL)
> -      validate_deref_var(instr->sampler, state);
> +      validate_deref_var(instr, instr->sampler, state);
>  }
>
>  static void
> @@ -438,10 +441,10 @@ validate_call_instr(nir_call_instr *instr,
validate_state *state)
>
>     for (unsigned i = 0; i < instr->num_params; i++) {
>        assert(instr->callee->params[i].type ==
instr->params[i]->deref.type);
> -      validate_deref_var(instr->params[i], state);
> +      validate_deref_var(instr, instr->params[i], state);
>     }
>
> -   validate_deref_var(instr->return_deref, state);
> +   validate_deref_var(instr, instr->return_deref, state);
>  }
>
>  static void
> diff --git a/src/mesa/program/prog_to_nir.c
b/src/mesa/program/prog_to_nir.c
> index 5f00a8b..b298d07 100644
> --- a/src/mesa/program/prog_to_nir.c
> +++ b/src/mesa/program/prog_to_nir.c
> @@ -153,8 +153,7 @@ ptn_get_src(struct ptn_compile *c, const struct
prog_src_register *prog_src)
>        nir_intrinsic_instr *load =
>           nir_intrinsic_instr_create(b->shader, nir_intrinsic_load_var);
>        load->num_components = 4;
> -      load->variables[0] =
> -         nir_deref_var_create(b->shader, c->input_vars[prog_src->Index]);
> +      load->variables[0] = nir_deref_var_create(load,
c->input_vars[prog_src->Index]);
>
>        nir_ssa_dest_init(&load->instr, &load->dest, 4, NULL);
>        nir_instr_insert_after_cf_list(b->cf_node_list, &load->instr);
> @@ -918,7 +917,7 @@ ptn_add_output_stores(struct ptn_compile *c)
>           nir_intrinsic_instr_create(b->shader, nir_intrinsic_store_var);
>        store->num_components = 4;
>        store->variables[0] =
> -         nir_deref_var_create(b->shader,
c->output_vars[var->data.location]);
> +         nir_deref_var_create(store, c->output_vars[var->data.location]);
>        store->src[0].reg.reg = c->output_regs[var->data.location];
>        nir_instr_insert_after_cf_list(c->build.cf_node_list,
&store->instr);
>     }
> @@ -962,7 +961,7 @@ setup_registers_and_variables(struct ptn_compile *c)
>              nir_intrinsic_instr *load_x =
>                 nir_intrinsic_instr_create(shader,
nir_intrinsic_load_var);
>              load_x->num_components = 1;
> -            load_x->variables[0] = nir_deref_var_create(shader, var);
> +            load_x->variables[0] = nir_deref_var_create(load_x, var);
>              nir_ssa_dest_init(&load_x->instr, &load_x->dest, 1, NULL);
>              nir_instr_insert_after_cf_list(b->cf_node_list,
&load_x->instr);
>
> @@ -978,7 +977,7 @@ setup_registers_and_variables(struct ptn_compile *c)
>              nir_intrinsic_instr *store =
>                 nir_intrinsic_instr_create(shader,
nir_intrinsic_store_var);
>              store->num_components = 4;
> -            store->variables[0] = nir_deref_var_create(shader, fullvar);
> +            store->variables[0] = nir_deref_var_create(store, fullvar);
>              store->src[0] = nir_src_for_ssa(f001);
>              nir_instr_insert_after_cf_list(b->cf_node_list,
&store->instr);
>
> --
> 2.3.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150407/8f82433b/attachment-0001.html>


More information about the mesa-dev mailing list