[Mesa-dev] [PATCH 20/21] nir/cf: add new control modification API's

[Kenneth Graunke]

On Tuesday, July 21, 2015 07:54:34 PM Connor Abbott wrote:
> These will help us do a number of things, including:
> 
> - Early return elimination.
> - Dead control flow elimination.
> - Various optimizations, such as replacing:
> 
> if (foo) {
>     ...
> }
> if (!foo) {
>     ...
> }
> 
> with:
> 
> if (foo) {
>     ...
> } else {
>     ...
> }
> 
> Signed-off-by: Connor Abbott <connor.w.abbott at intel.com>
> ---
>  src/glsl/nir/nir_control_flow.c | 62 ++++++++++++++++++++++++++++++++++
>  src/glsl/nir/nir_control_flow.h | 75 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 137 insertions(+)
> 
> diff --git a/src/glsl/nir/nir_control_flow.c b/src/glsl/nir/nir_control_flow.c
> index ba39205..7864138 100644
> --- a/src/glsl/nir/nir_control_flow.c
> +++ b/src/glsl/nir/nir_control_flow.c
> @@ -739,3 +739,65 @@ nir_cf_node_remove(nir_cf_node *node)
>     cleanup_cf_node(node, impl);
>  }
>  
> +void
> +nir_cf_extract(nir_cf_list *extracted, nir_cursor begin, nir_cursor end)
> +{
> +   nir_block *block_begin, *block_end, *block_before, *block_after;
> +
> +   /* In the case where begin points to an instruction in some basic block and
> +    * end points to the end of the same basic block, we rely on the fact that
> +    * splitting on an instruction moves earlier instructions into a new basic
> +    * block. If the later instructions were moved instead, then the end cursor
> +    * would be pointing to the same place that begin used to point to, which
> +    * is obviously not what we want.
> +    */
> +   split_block_cursor(begin, &block_before, &block_begin);
> +   split_block_cursor(end, &block_end, &block_after);
> +
> +   extracted->impl = nir_cf_node_get_function(&block_begin->cf_node);
> +   exec_list_make_empty(&extracted->list);
> +
> +   nir_cf_node *cf_node = &block_begin->cf_node;
> +   nir_cf_node *cf_node_end = &block_end->cf_node;
> +   while (true) {
> +      nir_cf_node *next = nir_cf_node_next(cf_node);
> +
> +      exec_node_remove(&cf_node->node);
> +      cf_node->parent = NULL;
> +      exec_list_push_tail(&extracted->list, &cf_node->node);
> +
> +      if (cf_node == cf_node_end)
> +         break;
> +
> +      cf_node = next;
> +   }
> +
> +   stitch_blocks(block_before, block_after);
> +}
> +
> +void
> +nir_cf_reinsert(nir_cf_list *cf_list, nir_cursor cursor)
> +{
> +   nir_block *before, *after;
> +
> +   split_block_cursor(cursor, &before, &after);
> +
> +   foreach_list_typed_safe(nir_cf_node, node, node, &cf_list->list) {
> +      exec_node_remove(&node->node);
> +      node->parent = before->cf_node.parent;
> +      exec_node_insert_node_before(&after->cf_node.node, &node->node);
> +   }
> +
> +   stitch_blocks(before,
> +                 nir_cf_node_as_block(nir_cf_node_next(&before->cf_node)));
> +   stitch_blocks(nir_cf_node_as_block(nir_cf_node_prev(&after->cf_node)),
> +                 after);
> +}
> +
> +void
> +nir_cf_delete(nir_cf_list *cf_list)
> +{
> +   foreach_list_typed(nir_cf_node, node, node, &cf_list->list) {
> +      cleanup_cf_node(node, cf_list->impl);
> +   }
> +}
> diff --git a/src/glsl/nir/nir_control_flow.h b/src/glsl/nir/nir_control_flow.h
> index 5a3be26..45aff3e 100644
> --- a/src/glsl/nir/nir_control_flow.h
> +++ b/src/glsl/nir/nir_control_flow.h
> @@ -37,6 +37,12 @@ extern "C" {
>   *
>   * This file contains various API's that make modifying control flow in NIR,
>   * while maintaining the invariants checked by the validator, much easier.
> + * There are two parts to this:
> + *
> + * 1. Inserting control flow (if's and loops) in various places, for creating
> + *    IR either from scratch or as part of some lowering pass.
> + * 2. Taking existing pieces of the IR and either moving them around or
> + *    deleting them.
>   */
>  
>  /* Helper struct for representing a point to extract/insert. Helps reduce the
> @@ -164,6 +170,75 @@ nir_cf_node_insert_end(struct exec_list *list, nir_cf_node *node)
>  /** removes a control flow node, doing any cleanup necessary */
>  void nir_cf_node_remove(nir_cf_node *node);
>  
> +/** Control flow motion.
> + *
> + * These functions let you take a part of a control flow list (basically
> + * equivalent to a series of statement in GLSL) and "extract" it from the IR,
> + * so that it's a free-floating piece of IR that can be either re-inserted
> + * somewhere else or deleted entirely. A few notes on using it:
> + *
> + * 1. Phi nodes are considered attached to the piece of control flow that
> + *    their sources come from. There are three places where phi nodes can
> + *    occur, which are the three places where a block can have multiple
> + *    predecessors:
> + *
> + *    1) After an if statement, if neither branch ends in a jump.
> + *    2) After a loop, if there are multiple break's.
> + *    3) At the beginning of a loop.
> + *
> + *    For #1, the phi node is considered to be part of the if, and for #2 and
> + *    #3 the phi node is considered to be part of the loop. This allows us to
> + *    keep phi's intact, but it means that phi nodes cannot be separated from
> + *    the control flow they come from. For example, extracting an if without
> + *    extracting all the phi nodes after it is not allowed, and neither is
> + *    extracting only some of the phi nodes at the beginning of a block. It
> + *    also means that extracting from the beginning of a basic block actually
> + *    means extracting from the first non-phi instruction, since there's no
> + *    situation where extracting phi nodes without extracting what comes
> + *    before them makes any sense.
> + *
> + * 2. Phi node sources are guaranteed to remain valid, meaning that they still
> + *    correspond one-to-one with the predecessors of the basic block they're
> + *    part of. In addition, the original sources will be preserved unless they
> + *    correspond to a break or continue that was deleted. However, no attempt
> + *    is made to ensure that SSA form is maintained. In particular, it is
> + *    *not* guaranteed that definitions of SSA values will dominate all their
> + *    uses after all is said and done. Either the caller must ensure that this
> + *    is the case, or it must insert extra phi nodes to restore SSA.
> + *
> + * 3. It is invalid to move a piece of IR with a break/continue outside of the
> + *    loop it references. Doing this will result in invalid
> + *    successors/predecessors and phi node sources.
> + *
> + * 4. It is invalid to move a piece of IR from one function implementation to
> + *    another.
> + *
> + * 5. Extracting a control flow list will leave lots of dangling references to
> + *    and from other pieces of the IR. It also leaves things in a not 100%
> + *    consistent state. This means that some things (e.g. inserting
> + *    instructions) might not work reliably on the extracted control flow. It
> + *    also means that extracting control flow without re-inserting it or
> + *    deleting it is a Bad Thing (tm).
> + */
> +
> +typedef struct {
> +   struct exec_list list;
> +   nir_function_impl *impl; /* for cleaning up if the list is deleted */
> +} nir_cf_list;
> +
> +void nir_cf_extract(nir_cf_list *extracted, nir_cursor begin, nir_cursor end);
> +
> +void nir_cf_reinsert(nir_cf_list *cf_list, nir_cursor cursor);
> +
> +void nir_cf_delete(nir_cf_list *cf_list);
> +
> +static inline void
> +nir_cf_list_extract(nir_cf_list *extracted, struct exec_list *cf_list)
> +{
> +   nir_cf_extract(extracted, nir_before_cf_list(cf_list),
> +                  nir_after_cf_list(cf_list));
> +}
> +
>  #ifdef __cplusplus
>  }
>  #endif
> 

Given all of these restrictions, I'd feel a lot better about using this
if nir_validate ensured that SSA is correct - phi definitions dominate
their sources, and there aren't dangling references to bogus blocks,
and so on.

This seems like a reasonable tool - one simply must exercise caution
when wielding it.  Which is fine, especially if we put some other
safeguards in place.  I like the simplicity - this is surprisingly
straightforward.  The cursor idea helps a lot as well.

I was wondering how it worked if there were no blocks, but then I
remembered that there are padding blocks after if statements and loops.
So that takes care of that.

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Feel free to land this series.

Now to review the control flow optimization series...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150805/10decc42/attachment.sig>