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

[Timothy Arceri]

On Fri, 2016-12-09 at 14:20 -0800, Jason Ekstrand wrote:
> On Mon, Dec 5, 2016 at 5:12 PM, Timothy Arceri <timothy.arceri at collab
> ora.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?

We exit if the src list is empty but the dst needs to at least have a
block to begin with to make clone_cf_list() happy. I'll add your
comment, thanks.

>  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
> > 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev