So, I was sort of imagining that we'd want to go a little farther with this, and make nir_instr_insert_before/after_* a wrapper around a cursor based API just called "nir_instr_insert". Then nir_builder_instr_insert() would become even simpler, since it would just call nir_instr_insert() and then point the cursor to after the instruction we just inserted (that part seems missing here, btw... what if I set the cursor to after an instruction and insert multiple instructions?) and it would be a lot more similar to how control flow modification works. It would also let us remove a little duplication from nir_instr_insert_* since we would only need to set the block and call add_defs_uses() once, and wrappers like nir_instr_insert_before_cf() would become a lot smaller with the cursor API handling most of the work. This could be a follow-on, but doing it before this patch would simplify the patch, so if you're willing to do the work...
On Thu, Aug 6, 2015 at 10:15 AM, Kenneth Graunke <[email protected]> wrote: > The NIR cursor API is exactly what we want for the builder's insertion > point. This simplifies the API, the implementation, and is actually > more flexible as well. > > This required a bit of reworking of TGSI->NIR's if/loop stack handling; > we now store cursors instead of cf_node_lists, for better or worse. > > Signed-off-by: Kenneth Graunke <[email protected]> > --- > src/gallium/auxiliary/nir/tgsi_to_nir.c | 34 +++++++------- > .../drivers/freedreno/ir3/ir3_nir_lower_if_else.c | 2 +- > src/gallium/drivers/vc4/vc4_nir_lower_io.c | 6 +-- > src/glsl/nir/nir_builder.h | 54 > ++++++++-------------- > src/glsl/nir/nir_lower_idiv.c | 2 +- > src/glsl/nir/nir_lower_load_const_to_scalar.c | 2 +- > src/glsl/nir/nir_lower_tex_projector.c | 2 +- > src/glsl/nir/nir_normalize_cubemap_coords.c | 2 +- > src/mesa/program/prog_to_nir.c | 2 +- > 9 files changed, 46 insertions(+), 60 deletions(-) > > I haven't tested the TGSI->NIR changes - I'm honestly not sure how. Could > someone try them out? Thanks! > > diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c > b/src/gallium/auxiliary/nir/tgsi_to_nir.c > index b17ac04..4485868 100644 > --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c > +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c > @@ -65,24 +65,24 @@ struct ttn_compile { > nir_register *addr_reg; > > /** > - * Stack of cf_node_lists where instructions should be pushed as we pop > + * Stack of nir_cursors where instructions should be pushed as we pop > * back out of the control flow stack. > * > * For each IF/ELSE/ENDIF block, if_stack[if_stack_pos] has where the else > * instructions should be placed, and if_stack[if_stack_pos - 1] has where > * the next instructions outside of the if/then/else block go. > */ > - struct exec_list **if_stack; > + nir_cursor *if_stack; > unsigned if_stack_pos; > > /** > - * Stack of cf_node_lists where instructions should be pushed as we pop > + * Stack of nir_cursors where instructions should be pushed as we pop > * back out of the control flow stack. > * > * loop_stack[loop_stack_pos - 1] contains the cf_node_list for the > outside > * of the loop. > */ > - struct exec_list **loop_stack; > + nir_cursor *loop_stack; > unsigned loop_stack_pos; > > /* How many TGSI_FILE_IMMEDIATE vec4s have been parsed so far. */ > @@ -922,7 +922,7 @@ ttn_if(struct ttn_compile *c, nir_ssa_def *src, bool > is_uint) > nir_builder *b = &c->build; > > /* Save the outside-of-the-if-statement node list. */ > - c->if_stack[c->if_stack_pos] = b->cf_node_list; > + c->if_stack[c->if_stack_pos] = b->cursor; > c->if_stack_pos++; > > src = ttn_channel(b, src, X); > @@ -933,11 +933,11 @@ ttn_if(struct ttn_compile *c, nir_ssa_def *src, bool > is_uint) > } else { > if_stmt->condition = nir_src_for_ssa(nir_fne(b, src, nir_imm_int(b, > 0))); > } > - nir_cf_node_insert_end(b->cf_node_list, &if_stmt->cf_node); > + nir_builder_cf_insert(b, &if_stmt->cf_node); > > - nir_builder_insert_after_cf_list(b, &if_stmt->then_list); > + b->cursor = nir_after_cf_list(&if_stmt->then_list); > > - c->if_stack[c->if_stack_pos] = &if_stmt->else_list; > + c->if_stack[c->if_stack_pos] = nir_after_cf_list(&if_stmt->else_list); > c->if_stack_pos++; > } > > @@ -946,7 +946,7 @@ ttn_else(struct ttn_compile *c) > { > nir_builder *b = &c->build; > > - nir_builder_insert_after_cf_list(b, c->if_stack[c->if_stack_pos - 1]); > + b->cursor = c->if_stack[c->if_stack_pos - 1]; > } > > static void > @@ -955,7 +955,7 @@ ttn_endif(struct ttn_compile *c) > nir_builder *b = &c->build; > > c->if_stack_pos -= 2; > - nir_builder_insert_after_cf_list(b, c->if_stack[c->if_stack_pos]); > + b->cursor = c->if_stack[c->if_stack_pos]; > } > > static void > @@ -964,13 +964,13 @@ ttn_bgnloop(struct ttn_compile *c) > nir_builder *b = &c->build; > > /* Save the outside-of-the-loop node list. */ > - c->loop_stack[c->loop_stack_pos] = b->cf_node_list; > + c->loop_stack[c->loop_stack_pos] = b->cursor; > c->loop_stack_pos++; > > nir_loop *loop = nir_loop_create(b->shader); > - nir_cf_node_insert_end(b->cf_node_list, &loop->cf_node); > + nir_builder_cf_insert(b, &loop->cf_node); > > - nir_builder_insert_after_cf_list(b, &loop->body); > + b->cursor = nir_after_cf_list(&loop->body); > } > > static void > @@ -993,7 +993,7 @@ ttn_endloop(struct ttn_compile *c) > nir_builder *b = &c->build; > > c->loop_stack_pos--; > - nir_builder_insert_after_cf_list(b, c->loop_stack[c->loop_stack_pos]); > + b->cursor = c->loop_stack[c->loop_stack_pos]; > } > > static void > @@ -1780,7 +1780,7 @@ tgsi_to_nir(const void *tgsi_tokens, > nir_function_impl *impl = nir_function_impl_create(overload); > > nir_builder_init(&c->build, impl); > - nir_builder_insert_after_cf_list(&c->build, &impl->body); > + c->build.cursor = nir_after_cf_list(&impl->body); > > tgsi_scan_shader(tgsi_tokens, &scan); > c->scan = &scan; > @@ -1799,10 +1799,10 @@ tgsi_to_nir(const void *tgsi_tokens, > c->num_samp_types = scan.file_max[TGSI_FILE_SAMPLER_VIEW] + 1; > c->samp_types = rzalloc_array(c, nir_alu_type, c->num_samp_types); > > - c->if_stack = rzalloc_array(c, struct exec_list *, > + c->if_stack = rzalloc_array(c, nir_cursor, > (scan.opcode_count[TGSI_OPCODE_IF] + > scan.opcode_count[TGSI_OPCODE_UIF]) * 2); > - c->loop_stack = rzalloc_array(c, struct exec_list *, > + c->loop_stack = rzalloc_array(c, nir_cursor, > scan.opcode_count[TGSI_OPCODE_BGNLOOP]); > > ret = tgsi_parse_init(&parser, tgsi_tokens); > diff --git a/src/gallium/drivers/freedreno/ir3/ir3_nir_lower_if_else.c > b/src/gallium/drivers/freedreno/ir3/ir3_nir_lower_if_else.c > index dc9e462..f5142b6 100644 > --- a/src/gallium/drivers/freedreno/ir3/ir3_nir_lower_if_else.c > +++ b/src/gallium/drivers/freedreno/ir3/ir3_nir_lower_if_else.c > @@ -171,7 +171,7 @@ flatten_block(nir_builder *bld, nir_block *if_block, > nir_block *prev_block, > (intr->intrinsic == > nir_intrinsic_discard_if)) { > nir_ssa_def *discard_cond; > > - nir_builder_insert_after_instr(bld, > + bld->cursor = nir_after_instr( > > nir_block_last_instr(prev_block)); > > if (invert) { > diff --git a/src/gallium/drivers/vc4/vc4_nir_lower_io.c > b/src/gallium/drivers/vc4/vc4_nir_lower_io.c > index ffc120e..8a87178 100644 > --- a/src/gallium/drivers/vc4/vc4_nir_lower_io.c > +++ b/src/gallium/drivers/vc4/vc4_nir_lower_io.c > @@ -59,7 +59,7 @@ vc4_nir_lower_input(struct vc4_compile *c, nir_builder *b, > /* All TGSI-to-NIR inputs are vec4. */ > assert(intr->num_components == 4); > > - nir_builder_insert_before_instr(b, &intr->instr); > + b->cursor = nir_before_instr(b, &intr->instr); > > nir_variable *input_var = NULL; > foreach_list_typed(nir_variable, var, node, &c->s->inputs) { > @@ -148,7 +148,7 @@ vc4_nir_lower_output(struct vc4_compile *c, nir_builder > *b, > /* All TGSI-to-NIR outputs are VEC4. */ > assert(intr->num_components == 4); > > - nir_builder_insert_before_instr(b, &intr->instr); > + b->cursor = nir_before_instr(&intr->instr); > > for (unsigned i = 0; i < intr->num_components; i++) { > nir_intrinsic_instr *intr_comp = > @@ -173,7 +173,7 @@ vc4_nir_lower_uniform(struct vc4_compile *c, nir_builder > *b, > /* All TGSI-to-NIR uniform loads are vec4. */ > assert(intr->num_components == 4); > > - nir_builder_insert_before_instr(b, &intr->instr); > + b->cursor = nir_before_instr(&intr->instr); > > /* Generate scalar loads equivalent to the original VEC4. */ > nir_ssa_def *dests[4]; > diff --git a/src/glsl/nir/nir_builder.h b/src/glsl/nir/nir_builder.h > index 9223e83..28f602a 100644 > --- a/src/glsl/nir/nir_builder.h > +++ b/src/glsl/nir/nir_builder.h > @@ -24,12 +24,12 @@ > #ifndef NIR_BUILDER_H > #define NIR_BUILDER_H > > +#include "nir_control_flow.h" > + > struct exec_list; > > typedef struct nir_builder { > - struct exec_list *cf_node_list; > - nir_instr *before_instr; > - nir_instr *after_instr; > + nir_cursor cursor; > > nir_shader *shader; > nir_function_impl *impl; > @@ -44,42 +44,28 @@ nir_builder_init(nir_builder *build, nir_function_impl > *impl) > } > > static inline void > -nir_builder_insert_after_cf_list(nir_builder *build, > - struct exec_list *cf_node_list) > -{ > - build->cf_node_list = cf_node_list; > - build->before_instr = NULL; > - build->after_instr = NULL; > -} > - > -static inline void > -nir_builder_insert_before_instr(nir_builder *build, nir_instr *before_instr) > -{ > - build->cf_node_list = NULL; > - build->before_instr = before_instr; > - build->after_instr = NULL; > -} > - > -static inline void > -nir_builder_insert_after_instr(nir_builder *build, nir_instr *after_instr) > +nir_builder_instr_insert(nir_builder *build, nir_instr *instr) > { > - build->cf_node_list = NULL; > - build->before_instr = NULL; > - build->after_instr = after_instr; > + switch (build->cursor.option) { > + case nir_cursor_before_block: > + nir_instr_insert_before_block(build->cursor.block, instr); > + break; > + case nir_cursor_after_block: > + nir_instr_insert_after_block(build->cursor.block, instr); > + break; > + case nir_cursor_before_instr: > + nir_instr_insert_before(build->cursor.instr, instr); > + break; > + case nir_cursor_after_instr: > + nir_instr_insert_after(build->cursor.instr, instr); > + break; > + } > } > > static inline void > -nir_builder_instr_insert(nir_builder *build, nir_instr *instr) > +nir_builder_cf_insert(nir_builder *build, nir_cf_node *cf) > { > - if (build->cf_node_list) { > - nir_instr_insert_after_cf_list(build->cf_node_list, instr); > - } else if (build->before_instr) { > - nir_instr_insert_before(build->before_instr, instr); > - } else { > - assert(build->after_instr); > - nir_instr_insert_after(build->after_instr, instr); > - build->after_instr = instr; > - } > + nir_cf_node_insert(build->cursor, cf); > } > > static inline nir_ssa_def * > diff --git a/src/glsl/nir/nir_lower_idiv.c b/src/glsl/nir/nir_lower_idiv.c > index 7b68032..0e1653d 100644 > --- a/src/glsl/nir/nir_lower_idiv.c > +++ b/src/glsl/nir/nir_lower_idiv.c > @@ -50,7 +50,7 @@ convert_instr(nir_builder *bld, nir_alu_instr *alu) > > is_signed = (op == nir_op_idiv); > > - nir_builder_insert_before_instr(bld, &alu->instr); > + bld->cursor = nir_before_instr(&alu->instr); > > numer = nir_ssa_for_src(bld, alu->src[0].src, > nir_ssa_alu_instr_src_components(alu, 0)); > diff --git a/src/glsl/nir/nir_lower_load_const_to_scalar.c > b/src/glsl/nir/nir_lower_load_const_to_scalar.c > index a90e524..b83ef05 100644 > --- a/src/glsl/nir/nir_lower_load_const_to_scalar.c > +++ b/src/glsl/nir/nir_lower_load_const_to_scalar.c > @@ -43,7 +43,7 @@ lower_load_const_instr_scalar(nir_load_const_instr *lower) > > nir_builder b; > nir_builder_init(&b, > nir_cf_node_get_function(&lower->instr.block->cf_node)); > - nir_builder_insert_before_instr(&b, &lower->instr); > + b.cursor = nir_before_instr(&lower->instr); > > /* Emit the individual loads. */ > nir_ssa_def *loads[4]; > diff --git a/src/glsl/nir/nir_lower_tex_projector.c > b/src/glsl/nir/nir_lower_tex_projector.c > index 357131c..8a482b1 100644 > --- a/src/glsl/nir/nir_lower_tex_projector.c > +++ b/src/glsl/nir/nir_lower_tex_projector.c > @@ -46,7 +46,7 @@ nir_lower_tex_projector_block(nir_block *block, void > *void_state) > continue; > > nir_tex_instr *tex = nir_instr_as_tex(instr); > - nir_builder_insert_before_instr(b, &tex->instr); > + b->cursor = nir_before_instr(&tex->instr); > > /* Find the projector in the srcs list, if present. */ > int proj_index; > diff --git a/src/glsl/nir/nir_normalize_cubemap_coords.c > b/src/glsl/nir/nir_normalize_cubemap_coords.c > index 0da8447..75b647f 100644 > --- a/src/glsl/nir/nir_normalize_cubemap_coords.c > +++ b/src/glsl/nir/nir_normalize_cubemap_coords.c > @@ -52,7 +52,7 @@ normalize_cubemap_coords_block(nir_block *block, void > *void_state) > if (tex->sampler_dim != GLSL_SAMPLER_DIM_CUBE) > continue; > > - nir_builder_insert_before_instr(b, &tex->instr); > + b->cursor = nir_before_instr(&tex->instr); > > for (unsigned i = 0; i < tex->num_srcs; i++) { > if (tex->src[i].src_type != nir_tex_src_coord) > diff --git a/src/mesa/program/prog_to_nir.c b/src/mesa/program/prog_to_nir.c > index 29e0c19..d6eb13b 100644 > --- a/src/mesa/program/prog_to_nir.c > +++ b/src/mesa/program/prog_to_nir.c > @@ -1102,7 +1102,7 @@ prog_to_nir(const struct gl_program *prog, > > c->build.shader = s; > c->build.impl = impl; > - nir_builder_insert_after_cf_list(&c->build, &impl->body); > + c->build.cursor = nir_after_cf_list(&impl->body); > > setup_registers_and_variables(c); > if (unlikely(c->error)) > -- > 2.4.6 > > _______________________________________________ > mesa-dev mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
