On Mon, Aug 27, 2018 at 4:55 PM Caio Marcelo de Oliveira Filho <
caio.olive...@intel.com> wrote:
> (Disregard the incomplete mail, still adapting to notmuch-emacs).
>
> Jason Ekstrand <ja...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
