[Mesa-dev] [PATCH 08/13] nir: add control flow helpers for loop unrolling

[Connor Abbott]

On Mon, Aug 29, 2016 at 11:57 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> On Mon, 2016-08-29 at 20:42 -0400, Connor Abbott wrote:
>> I already noted in patch 12/13 that you can get rid of your use of
>> stitch_blocks(). I also don't get why you need a special
>> nir_cf_loop_list_extract() here... why
>>
>> On Mon, Aug 29, 2016 at 12:59 AM, Timothy Arceri
>> <timothy.arceri at collabora.com> wrote:
>> >
>> > This makes stitch_blocks() available for use else where, and adds
>> > a new helper that extracts a cf list without worrying about
>> > validation.
>>
>> I already noted in patch 12/13 that you can get rid of your use of
>> stitch_blocks(). I also don't get why you need a special
>> nir_cf_loop_list_extract() here... why can't you use
>> nir_cf_extract()?
>> The only difference between the two is that nir_cf_extract()
>> re-stitches the nodes together, and it also makes sure that you don't
>> handle phi nodes incorrectly, but AFAICT those differences don't
>> matter for the way you intend to use it.
>
> As I said manipulating the control flow for loops in is not much fun.
> The current cf helpers are not very well suited to loop unrolling
> because they try to be too smart keeping the cf in a valid state as we
> go but extracting almost anything from a loop can break it.
>
> nir_cf_extract() will fall over if we try to use it.

How does it fall over? It's supposed to handle what you're doing
correctly. The keeping things in a valid state part is intentional,
since it keeps you from shooting yourself in the foot. That's not
supposed to preclude you from doing stuff like this, though.

(We haven't been doing a lot of serious control-flow munging, so I
wouldn't be surprised if you ran into some bugs in the control flow
modification code... it's pretty tricky!)

>
>>
>> >
>> > ---
>> >  src/compiler/nir/nir_control_flow.c | 34
>> > ++++++++++++++++++++++++++++++++--
>> >  src/compiler/nir/nir_control_flow.h |  5 +++++
>> >  2 files changed, 37 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/src/compiler/nir/nir_control_flow.c
>> > b/src/compiler/nir/nir_control_flow.c
>> > index a485e71..ed8cd24 100644
>> > --- a/src/compiler/nir/nir_control_flow.c
>> > +++ b/src/compiler/nir/nir_control_flow.c
>> > @@ -628,8 +628,7 @@ update_if_uses(nir_cf_node *node)
>> >   * Stitch two basic blocks together into one. The aggregate must
>> > have the same
>> >   * predecessors as the first and the same successors as the
>> > second.
>> >   */
>> > -
>> > -static void
>> > +void
>> >  stitch_blocks(nir_block *before, nir_block *after)
>> >  {
>> >     /*
>> > @@ -791,6 +790,37 @@ nir_cf_extract(nir_cf_list *extracted,
>> > nir_cursor begin, nir_cursor end)
>> >     stitch_blocks(block_before, block_after);
>> >  }
>> >
>> > +/**
>> > + * Its not really possible to extract control flow from a loop
>> > while keeping
>> > + * the cf valid so this function just rips out what we ask for and
>> > any
>> > + * validation and fix ups are left to the caller.
>> > + */
>> > +void
>> > +nir_cf_loop_list_extract(nir_cf_list *extracted, nir_cf_node
>> > *begin,
>> > +                         nir_cf_node *end)
>> > +{
>> > +   extracted->impl = nir_cf_node_get_function(begin);
>> > +   exec_list_make_empty(&extracted->list);
>> > +
>> > +   /* Dominance and other block-related information is toast. */
>> > +   nir_metadata_preserve(extracted->impl, nir_metadata_none);
>> > +
>> > +   nir_cf_node *cf_node = begin;
>> > +   nir_cf_node *cf_node_end = end;
>> > +   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;
>> > +   }
>> > +}
>> > +
>> >  void
>> >  nir_cf_reinsert(nir_cf_list *cf_list, nir_cursor cursor)
>> >  {
>> > diff --git a/src/compiler/nir/nir_control_flow.h
>> > b/src/compiler/nir/nir_control_flow.h
>> > index b71382f..0d97486 100644
>> > --- a/src/compiler/nir/nir_control_flow.h
>> > +++ b/src/compiler/nir/nir_control_flow.h
>> > @@ -78,6 +78,9 @@ nir_cf_node_insert_end(struct exec_list *list,
>> > nir_cf_node *node)
>> >     nir_cf_node_insert(nir_after_cf_list(list), node);
>> >  }
>> >
>> > +void
>> > +stitch_blocks(nir_block *before, nir_block *after);
>> > +
>> >
>> >  /** Control flow motion.
>> >   *
>> > @@ -148,6 +151,8 @@ nir_cf_list_extract(nir_cf_list *extracted,
>> > struct exec_list *cf_list)
>> >                    nir_after_cf_list(cf_list));
>> >  }
>> >
>> > +void nir_cf_loop_list_extract(nir_cf_list *extracted, nir_cf_node
>> > *begin, nir_cf_node *end);
>> > +
>> >  /** removes a control flow node, doing any cleanup necessary */
>> >  static inline void
>> >  nir_cf_node_remove(nir_cf_node *node)
>> > --
>> > 2.7.4
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev