[Mesa-dev] [PATCH 3/3] nir: add the ability insert a CF node after an instruction

[Jason Ekstrand]

On Jul 16, 2015 5:19 PM, "Kenneth Graunke" <kenneth at whitecape.org> wrote:
>
> From: Connor Abbott <connor.w.abbott at intel.com>
>
> 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 <jason at jlekstrand.net>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150716/7e397b32/attachment-0001.html>