[Mesa-dev] [PATCH 07/10] nir: add helper for cloning nir_cf_list

[Jason Ekstrand]

On Mon, Dec 5, 2016 at 5:12 PM, Timothy Arceri <timothy.arceri at collabora.com
> wrote:

> V2:
> - updated to create a generic list clone helper nir_cf_list_clone()
> - continue to assert on clone when fallback flag not set as suggested
>   by Jason.
> ---
>  src/compiler/nir/nir_clone.c        | 58 ++++++++++++++++++++++++++++++
> +------
>  src/compiler/nir/nir_control_flow.h |  3 ++
>  2 files changed, 52 insertions(+), 9 deletions(-)
>
> diff --git a/src/compiler/nir/nir_clone.c b/src/compiler/nir/nir_clone.c
> index e6483b1..b9b7829 100644
> --- a/src/compiler/nir/nir_clone.c
> +++ b/src/compiler/nir/nir_clone.c
> @@ -22,7 +22,7 @@
>   */
>
>  #include "nir.h"
> -#include "nir_control_flow_private.h"
> +#include "nir_control_flow.h"
>
>  /* Secret Decoder Ring:
>   *   clone_foo():
> @@ -35,6 +35,11 @@ typedef struct {
>     /* True if we are cloning an entire shader. */
>     bool global_clone;
>
> +   /* This allows us to clone a loop body without having to add srcs from
> +    * outside the loop to the remap table. This is useful for loop
> unrolling.
>

It would be better of this comment started with a description of what the
variable means rather than a use-case.  The other is fine, but we should
say some thing such as "allow the clone operation to fall back to the
original pointer if no clone pointer is found in the remap table."


> +    */
> +   bool allow_remap_fallback;
> +
>     /* maps orig ptr -> cloned ptr: */
>     struct hash_table *remap_table;
>
> @@ -46,11 +51,19 @@ typedef struct {
>  } clone_state;
>
>  static void
> -init_clone_state(clone_state *state, bool global)
> +init_clone_state(clone_state *state, struct hash_table *remap_table,
> +                 bool global, bool allow_remap_fallback)
>  {
>     state->global_clone = global;
> -   state->remap_table = _mesa_hash_table_create(NULL, _mesa_hash_pointer,
> -                                                _mesa_key_pointer_equal);
> +   state->allow_remap_fallback = allow_remap_fallback;
> +
> +   if (remap_table) {
> +      state->remap_table = remap_table;
> +   } else {
> +      state->remap_table = _mesa_hash_table_create(NULL,
> _mesa_hash_pointer,
> +
>  _mesa_key_pointer_equal);
> +   }
> +
>     list_inithead(&state->phi_srcs);
>  }
>
> @@ -72,9 +85,10 @@ _lookup_ptr(clone_state *state, const void *ptr, bool
> global)
>        return (void *)ptr;
>
>     entry = _mesa_hash_table_search(state->remap_table, ptr);
> -   assert(entry && "Failed to find pointer!");
> -   if (!entry)
> -      return NULL;
> +   if (!entry) {
> +      assert(state->allow_remap_fallback);
> +      return (void *)ptr;
> +   }
>
>     return entry->data;
>  }
> @@ -613,6 +627,32 @@ fixup_phi_srcs(clone_state *state)
>     assert(list_empty(&state->phi_srcs));
>  }
>
> +void
> +nir_cf_list_clone(nir_cf_list *dst, nir_cf_list *src, nir_cf_node *parent,
> +                  struct hash_table *remap_table)
> +{
> +   exec_list_make_empty(&dst->list);
> +   dst->impl = src->impl;
> +
> +   if (exec_list_is_empty(&src->list))
> +      return;
> +
> +   clone_state state;
> +   init_clone_state(&state, remap_table, false, true);
> +
> +   /* We use the same shader */
> +   state.ns = src->impl->function->shader;
> +
> +   /* Dest list needs to at least have one block */
>

I'm confused by this.  If src->list is empty, then we'll bale above and
never get here.  Or is this just so that the control-flow code will be
happy?  If that's the case, perhaps we should say something like "The
control-flow code assumes that the list of cf_nodes always starts and ends
with a block.  We start by adding an empty block."

With those two comments addressed, 6 and 7 are

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

This is much nicer.  Thank you!


> +   nir_block *nblk = nir_block_create(state.ns);
> +   nblk->cf_node.parent = parent;
> +   exec_list_push_tail(&dst->list, &nblk->cf_node.node);
>
+
> +   clone_cf_list(&state, &dst->list, &src->list);
> +
> +   fixup_phi_srcs(&state);
> +}
> +
>  static nir_function_impl *
>  clone_function_impl(clone_state *state, const nir_function_impl *fi)
>  {
> @@ -646,7 +686,7 @@ nir_function_impl *
>  nir_function_impl_clone(const nir_function_impl *fi)
>  {
>     clone_state state;
> -   init_clone_state(&state, false);
> +   init_clone_state(&state, NULL, false, false);
>
>     /* We use the same shader */
>     state.ns = fi->function->shader;
> @@ -686,7 +726,7 @@ nir_shader *
>  nir_shader_clone(void *mem_ctx, const nir_shader *s)
>  {
>     clone_state state;
> -   init_clone_state(&state, true);
> +   init_clone_state(&state, NULL, true, false);
>
>     nir_shader *ns = nir_shader_create(mem_ctx, s->stage, s->options,
> NULL);
>     state.ns = ns;
> diff --git a/src/compiler/nir/nir_control_flow.h b/src/compiler/nir/nir_
> control_flow.h
> index b71382f..b496aec 100644
> --- a/src/compiler/nir/nir_control_flow.h
> +++ b/src/compiler/nir/nir_control_flow.h
> @@ -141,6 +141,9 @@ void nir_cf_reinsert(nir_cf_list *cf_list, nir_cursor
> cursor);
>
>  void nir_cf_delete(nir_cf_list *cf_list);
>
> +void nir_cf_list_clone(nir_cf_list *dst, nir_cf_list *src, nir_cf_node
> *parent,
> +                       struct hash_table *remap_table);
> +
>  static inline void
>  nir_cf_list_extract(nir_cf_list *extracted, struct exec_list *cf_list)
>  {
> --
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161209/49cc321f/attachment-0001.html>