Other than my nitpicking below this looks great! Thanks for working on this! On Apr 7, 2015 2:32 AM, "Kenneth Graunke" <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> 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 <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > --- > 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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>