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

[Connor Abbott]

On Thu, Jul 16, 2015 at 3:18 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.

I went ahead and pushed the first two patches to master with your and
Jason's r-b. I'm starting to work on control flow modification,
though, and part of that means a lot of shuffling things around. I'd
rather add this functionality after the shuffling, especially since
the helper I added may intersect with some other stuff we have to add,
and I'd rather wait until we see the end result before adding it -- I
may wind up refactoring it, in which case this patch might look
different. Hopefully we should be done with this soon, but in the
meantime you can carry this patch around.

Connor

>
> 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) {
> +      exec_node_remove(&cur_instr->node);
> +      exec_list_push_tail(&new_block->instr_list, &cur_instr->node);
> +      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
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev