[Mesa-dev] [PATCH 1/6] nir/dead_variables: Remove shader-local variables that are only written

[Jason Ekstrand]

On Wed, Jan 4, 2017 at 3:20 PM, Timothy Arceri <timothy.arceri at collabora.com
> wrote:

> On Mon, 2016-12-12 at 19:39 -0800, Jason Ekstrand wrote:
> > ---
> >  src/compiler/nir/nir_remove_dead_variables.c | 66
> > ++++++++++++++++++++++++----
> >  1 file changed, 58 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/compiler/nir/nir_remove_dead_variables.c
> > b/src/compiler/nir/nir_remove_dead_variables.c
> > index f7429eb..d22b7f5 100644
> > --- a/src/compiler/nir/nir_remove_dead_variables.c
> > +++ b/src/compiler/nir/nir_remove_dead_variables.c
> > @@ -31,9 +31,27 @@ static void
> >  add_var_use_intrinsic(nir_intrinsic_instr *instr, struct set *live)
> >  {
> >     unsigned num_vars = nir_intrinsic_infos[instr-
> > >intrinsic].num_variables;
> > -   for (unsigned i = 0; i < num_vars; i++) {
> > -      nir_variable *var = instr->variables[i]->var;
> > -      _mesa_set_add(live, var);
> > +
> > +   switch (instr->intrinsic) {
> > +   case nir_intrinsic_copy_var:
> > +      _mesa_set_add(live, instr->variables[1]->var);
> > +      /* Fall through */
> > +   case nir_intrinsic_store_var: {
> > +      /* The first source in both copy_var and store_var is the
> > destination.
> > +       * If the variable is a local that never escapes the shader,
> > then we
> > +       * don't mark it as live for just a store.
> > +       */
> > +      nir_variable_mode mode = instr->variables[0]->var->data.mode;
> > +      if (!(mode & (nir_var_local | nir_var_global |
> > nir_var_shared)))
>
> So you have nir_var_shared here but I think you are missing the bit to
> add:
>
>    if (modes & nir_var_shared)
>       progress = remove_dead_vars(&shader->shared, live) || progress;
>

That needs to be its own patch, but sure.


>
> Otherwise won't the var remain while the write instruction is removed?
>
>
> > +         _mesa_set_add(live, instr->variables[0]->var);
> > +      break;
> > +   }
> > +
> > +   default:
> > +      for (unsigned i = 0; i < num_vars; i++) {
> > +         _mesa_set_add(live, instr->variables[i]->var);
> > +      }
> > +      break;
> >     }
> >  }
> >
> > @@ -94,6 +112,31 @@ add_var_use_shader(nir_shader *shader, struct set
> > *live)
> >     }
> >  }
> >
> > +static void
> > +remove_dead_var_writes(nir_shader *shader, struct set *live)
> > +{
> > +   nir_foreach_function(function, shader) {
> > +      if (!function->impl)
> > +         continue;
> > +
> > +      nir_foreach_block(block, function->impl) {
> > +         nir_foreach_instr_safe(instr, block) {
> > +            if (instr->type != nir_instr_type_intrinsic)
> > +               continue;
> > +
> > +            nir_intrinsic_instr *intrin =
> > nir_instr_as_intrinsic(instr);
> > +            if (intrin->intrinsic != nir_intrinsic_copy_var &&
> > +                intrin->intrinsic != nir_intrinsic_store_var)
> > +               continue;
> > +
> > +            /* Stores to dead variables need to be removed */
> > +            if (!_mesa_set_search(live, intrin->variables[0]->var))
> > +               nir_instr_remove(instr);
> > +         }
> > +      }
> > +   }
> > +}
> > +
> >  static bool
> >  remove_dead_vars(struct exec_list *var_list, struct set *live)
> >  {
> > @@ -138,12 +181,19 @@ nir_remove_dead_variables(nir_shader *shader,
> > nir_variable_mode modes)
> >     if (modes & nir_var_local) {
> >        nir_foreach_function(function, shader) {
> >           if (function->impl) {
> > -            if (remove_dead_vars(&function->impl->locals, live)) {
> > -               nir_metadata_preserve(function->impl,
> > nir_metadata_block_index |
> > -                                                     nir_metadata_do
> > minance |
> > -                                                     nir_metadata_li
> > ve_ssa_defs);
> > +            if (remove_dead_vars(&function->impl->locals, live))
> >                 progress = true;
> > -            }
> > +         }
> > +      }
> > +   }
> > +
> > +   if (progress) {
> > +      remove_dead_var_writes(shader, live);
>
> The vars are conditional on modes passed to nir_remove_dead_variables()
> dont we need this here too? Otherwise we will remove the write
> instruction but not the var.
>

Hrm... Yes.  I'm thinking that we probably want to set the variable mode to
0 when we delete it and use that to decide when to remove a write.  That
way we never run into this problem in the future.


> > +
> > +      nir_foreach_function(function, shader) {
> > +         if (function->impl) {
> > +            nir_metadata_preserve(function->impl,
> > nir_metadata_block_index |
> > +                                                  nir_metadata_domin
> > ance);
> >           }
> >        }
> >     }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170106/35392883/attachment-0001.html>