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