Other than my nitpicking below this looks great! Thanks for working on this!
On Apr 7, 2015 2:32 AM, "Kenneth Graunke" <kenn...@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 <kenn...@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(©_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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
