On Jul 16, 2015 5:19 PM, "Kenneth Graunke" <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> wrote: > > From: Connor Abbott <<a href="mailto:connor.w.abbott@intel.com">connor.w.abbott@intel.com</a>> > > This will split the block containing the instruction and put the CF node > in between. > > v2: (by Kenneth Graunke) > - Simplify split_block_after_instr()'s implementation by using > split_block_end() rather than duplicating code. > - Fix a bug in nir_cf_node_insert_after_instr() where inserting a > non-block after the last instruction would cause update_if_uses() > to be called twice, making us try to add the same SSA def to the > if_uses list twice, corrupting the list. > - Comment changes. > > Cc: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > --- > src/glsl/nir/nir.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > src/glsl/nir/nir.h | 3 +++ > 2 files changed, 65 insertions(+) > > Nothing uses this yet, but I've tested it with my SIMD8 geometry shader patches, > which use this to replace emit_vertex intrinsics with if blocks (for "safety > checks" that make sure the program hasn't emitted too many vertices). It seems > to work just fine, and seems like a really useful piece of infrastructure to > have, so I'm submitting it now. > > Jason, would you mind reviewing it, since Connor and I both hacked on it? > It would be nice to have a non-author take a look at it :) > > diff --git a/src/glsl/nir/nir.c b/src/glsl/nir/nir.c > index 78ff886..0c53bab 100644 > --- a/src/glsl/nir/nir.c > +++ b/src/glsl/nir/nir.c > @@ -843,6 +843,29 @@ split_block_end(nir_block *block) > } > > /** > + * Creates a new block, and moves all the instructions after the given > + * instruction to the new block. > + */ > +static nir_block * > +split_block_after_instr(nir_instr *instr) > +{ > + /* We don't have to do anything special for handling jump instructions, > + * as this will move the successors associated with the jump to the new > + * block already. > + */ > + nir_block *new_block = split_block_end(instr->block); > + > + nir_instr *cur_instr; > + while ((cur_instr = nir_instr_next(instr)) != NULL) { Please use a for loop or pull the iteration expression out. This is really obtuse. Otherwise, at first glance this looks pretty good. I'd like to take a longer look before I call it reviewed though. > + exec_node_remove(&cur_instr->node); > + exec_list_push_tail(&new_block->instr_list, &cur_instr->node); At some point we should get a better mechanism for list splicing. Don't bother with it now because I'm hoping to move NIR to the list in until before too long. We can make the change then. > + cur_instr->block = new_block; > + } > + > + return new_block; > +} > + > +/** > * Inserts a non-basic block between two basic blocks and links them together. > */ > > @@ -1124,6 +1147,45 @@ nir_cf_node_insert_after(nir_cf_node *node, nir_cf_node *after) > } > > void > +nir_cf_node_insert_after_instr(nir_instr *instr, nir_cf_node *after) > +{ > + /* If the instruction is the last in its block, then this is equivalent > + * to inserting the CF node after this block. Just call that, to avoid > + * attempting to split blocks unnecessarily. > + */ > + if (nir_instr_is_last(instr)) { > + nir_cf_node_insert_after(&instr->block->cf_node, after); > + return; > + } > + > + update_if_uses(after); > + > + if (after->type == nir_cf_node_block) { > + /* We're attempting to insert a block after an instruction; instead, > + * just move all of the instructions into the existing block. Actually > + * removing and adding them would involve removing and adding uses/defs, > + * which we don't need to do, so just take them off the list directly. > + */ > + nir_block *after_block = nir_cf_node_as_block(after); > + nir_foreach_instr_safe_reverse(after_block, new_instr) { > + exec_node_remove(&new_instr->node); > + new_instr->block = instr->block; > + exec_node_insert_after(&instr->node, &new_instr->node); > + } > + } else { > + /* We're inserting a loop or if after an instruction. Split up the > + * basic block and insert it between those two blocks. > + */ > + nir_block *before_block = instr->block; > + nir_block *after_block = split_block_after_instr(instr); > + insert_non_block(before_block, after, after_block); > + } > + > + nir_function_impl *impl = nir_cf_node_get_function(&instr->block->cf_node); > + nir_metadata_preserve(impl, nir_metadata_none); > +} > + > +void > nir_cf_node_insert_before(nir_cf_node *node, nir_cf_node *before) > { > update_if_uses(before); > diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h > index 62cdbd4..6efbc18 100644 > --- a/src/glsl/nir/nir.h > +++ b/src/glsl/nir/nir.h > @@ -1506,6 +1506,9 @@ void nir_cf_node_insert_after(nir_cf_node *node, nir_cf_node *after); > /** puts a control flow node immediately before another control flow node */ > void nir_cf_node_insert_before(nir_cf_node *node, nir_cf_node *before); > > +/** puts a control flow node immediately after a given instruction */ > +void nir_cf_node_insert_after_instr(nir_instr *instr, nir_cf_node *after); > + > /** puts a control flow node at the beginning of a list from an if, loop, or function */ > void nir_cf_node_insert_begin(struct exec_list *list, nir_cf_node *node); > > -- > 2.4.5 >