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

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

2016-08-30 Thread Connor Abbott

On Mon, Aug 29, 2016 at 11:57 PM, Timothy Arceri
 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
>>  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(>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(_node->node);
>> > +  cf_node->parent = NULL;
>> > +  exec_list_push_tail(>list, _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@lists.freedesktop.org

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

2016-08-29 Thread Timothy Arceri

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
>  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.

> 
> > 
> > ---
> >  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(>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(_node->node);
> > +  cf_node->parent = NULL;
> > +  exec_list_push_tail(>list, _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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-08-29 Thread Connor Abbott

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
 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.

> ---
>  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(>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(_node->node);
> +  cf_node->parent = NULL;
> +  exec_list_push_tail(>list, _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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-08-28 Thread Timothy Arceri

This makes stitch_blocks() available for use else where, and adds
a new helper that extracts a cf list without worrying about
validation.
---
 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(>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(_node->node);
+  cf_node->parent = NULL;
+  exec_list_push_tail(>list, _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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information