[Mesa-dev] [PATCH 5/9] nir: Add a local dead write vars removal pass

[Jason Ekstrand]

On Mon, Aug 27, 2018 at 4:55 PM Caio Marcelo de Oliveira Filho <
caio.oliveira at intel.com> wrote:

> (Disregard the incomplete mail, still adapting to notmuch-emacs).
>
> Jason Ekstrand <jason at jlekstrand.net> writes:
>
> >> +static nir_deref_path *
> >> +get_path(struct state *state, nir_deref_instr *deref)
> >> +{
> >> +   struct hash_entry *entry = _mesa_hash_table_search(state->paths,
> >> deref);
> >> +   if (!entry) {
> >> +      nir_deref_path *path = linear_zalloc_child(state->path_lin_ctx,
> >> sizeof(nir_deref_path));
> >> +      nir_deref_path_init(path, deref, state->mem_ctx);
> >> +      _mesa_hash_table_insert(state->paths, deref, path);
> >> +      return path;
> >> +   } else {
> >> +      return entry->data;
> >> +   }
> >> +}
> >>
> >
> > Do you have any proof that this actually helps?  The deref_path stuff was
> > designed to be put on the stack and absolutely as efficient as possible.
> > In the common case of a deref chain with only a couple of elements, I
> would
> > expect to actually be more work to look it up in a hash table.
>
> Storing those makes more sense if you read the next commit (the
> "global").  Since I've created the "local" commit as an aid for
> reviewing (perhaps a failure), I did not wanted to change it too much.
>
> When I wrote there were some savings when measuring executions with
> perf.
>
> (...)
>
> >> +static bool
> >> +remove_dead_write_vars_local(struct state *state, nir_block *block)
> >> +{
> >> +   bool progress = false;
> >> +
> >> +   struct util_dynarray unused_writes;
> >> +   util_dynarray_init(&unused_writes, state->mem_ctx);
> >> +
> >> +   nir_foreach_instr_safe(instr, block) {
> >>
> >
> > It wouldn't hurt to add a case for call instructions which does a barrier
> > on everything I mentioned below as well as globals and locals.
>
> Makes sense.  But I don't get locals are affect?  Is this to cover the
> parameters being passed to the call?
>

Because a deref to a local might be passed in as a parameter.  This is the
way pass-by-reference works for SPIR-V.


> >
> >> +      if (instr->type != nir_instr_type_intrinsic)
> >> +         continue;
> >> +
> >> +      nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);
> >> +      switch (intrin->intrinsic) {
> >> +      case nir_intrinsic_barrier:
> >> +      case nir_intrinsic_memory_barrier: {
> >> +         nir_variable_mode modes = ~(nir_var_local | nir_var_global |
> >> +                                     nir_var_shader_in |
> nir_var_uniform);
> >>
> >
> > The only thing a barrier like this affects is shared, storage, and
> output.
> > Locals and globals can't cross between shader channels so there's no
> reason
> > to do anything with them on a barrier.  For inputs and uniforms, they're
> > never written anyway so there's no point in doing anything with them on a
> > barrier.
>
> This came from previous code, but except for "system values" it seems to
> do the right thing (note the ~).  Is the suggestion to use a "positive"
> enumeration, e.g.
>

Drp... I completely missed the ~.  Ignore my comment.  Yeah, we could throw
in system values. :D
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180828/52294b7c/attachment-0001.html>