On Wed, Aug 15, 2018 at 4:57 PM Caio Marcelo de Oliveira Filho < caio.olive...@intel.com> wrote:
> Instead of doing this as part of the existing (local) copy prop vars > pass. This is an intermediate step before changing both the dead > write and the copy prop vars to act on the whole program instead of on > local blocks. The nature of data we store and the way we iterate is > different enough that would be awkward keeping those together. > --- > src/compiler/Makefile.sources | 1 + > src/compiler/nir/meson.build | 1 + > src/compiler/nir/nir.h | 2 + > src/compiler/nir/nir_opt_dead_write_vars.c | 243 +++++++++++++++++++++ > 4 files changed, 247 insertions(+) > create mode 100644 src/compiler/nir/nir_opt_dead_write_vars.c > > diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources > index 27a54e0be09..fa93ad08a16 100644 > --- a/src/compiler/Makefile.sources > +++ b/src/compiler/Makefile.sources > @@ -274,6 +274,7 @@ NIR_FILES = \ > nir/nir_opt_cse.c \ > nir/nir_opt_dce.c \ > nir/nir_opt_dead_cf.c \ > + nir/nir_opt_dead_write_vars.c \ > nir/nir_opt_gcm.c \ > nir/nir_opt_global_to_local.c \ > nir/nir_opt_if.c \ > diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build > index 8708f9b069c..1c164a548a7 100644 > --- a/src/compiler/nir/meson.build > +++ b/src/compiler/nir/meson.build > @@ -158,6 +158,7 @@ files_libnir = files( > 'nir_opt_cse.c', > 'nir_opt_dce.c', > 'nir_opt_dead_cf.c', > + 'nir_opt_dead_write_vars.c', > 'nir_opt_gcm.c', > 'nir_opt_global_to_local.c', > 'nir_opt_if.c', > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > index d0fa693884b..becf6e351c3 100644 > --- a/src/compiler/nir/nir.h > +++ b/src/compiler/nir/nir.h > @@ -2968,6 +2968,8 @@ bool nir_opt_dce(nir_shader *shader); > > bool nir_opt_dead_cf(nir_shader *shader); > > +bool nir_opt_dead_write_vars(nir_shader *shader); > + > bool nir_opt_gcm(nir_shader *shader, bool value_number); > > bool nir_opt_if(nir_shader *shader); > diff --git a/src/compiler/nir/nir_opt_dead_write_vars.c > b/src/compiler/nir/nir_opt_dead_write_vars.c > new file mode 100644 > index 00000000000..822bfa5595d > --- /dev/null > +++ b/src/compiler/nir/nir_opt_dead_write_vars.c > @@ -0,0 +1,243 @@ > +/* > + * Copyright © 2018 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the > "Software"), > + * to deal in the Software without restriction, including without > limitation > + * the rights to use, copy, modify, merge, publish, distribute, > sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the > next > + * paragraph) shall be included in all copies or substantial portions of > the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS > + * IN THE SOFTWARE. > + */ > + > +#include "nir.h" > +#include "nir_builder.h" > +#include "nir_deref.h" > + > +#include "util/u_dynarray.h" > + > +struct state { > + void *mem_ctx; > + > + /* Maps nir_deref_instr to a corresponding nir_deref_path. Avoids > + * rebuilding the paths for the same deref. */ > + struct hash_table *paths; > + void *path_lin_ctx; > +}; > + > +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. > + > +/* Entry for unused_writes arrays. */ > +struct write_entry { > + /* If NULL indicates the entry is free to be reused. */ > + nir_intrinsic_instr *intrin; > + uintptr_t mask; > + nir_deref_path *path; > +}; > + > +static void > +clear_unused_for_modes(struct util_dynarray *unused_writes, > nir_variable_mode modes) > +{ > + util_dynarray_foreach(unused_writes, struct write_entry, entry) { > + if (!entry->intrin) > + continue; > + nir_variable *var = entry->path->path[0]->var; > + if (var->data.mode & modes) > + entry->intrin = NULL; > + } > +} > + > +static void > +clear_unused_for_src(struct util_dynarray *unused_writes, nir_deref_path > *src_path) > +{ > + util_dynarray_foreach(unused_writes, struct write_entry, entry) { > + if (!entry->intrin) > + continue; > + if (nir_compare_deref_paths(src_path, entry->path) & > nir_derefs_may_alias_bit) > + entry->intrin = NULL; > + } > +} > + > +static bool > +update_unused_writes_with_dst(struct util_dynarray *unused_writes, > + nir_intrinsic_instr *intrin, > + nir_deref_path *dst_path, uintptr_t mask) > +{ > + /* If we see an empty entry, keep track of it for reuse later. */ > + struct write_entry *empty_entry = NULL; > + bool progress = false; > + > + /* Find writes that are unused and can be removed. */ > + util_dynarray_foreach(unused_writes, struct write_entry, entry) { > + if (!entry->intrin) { > + empty_entry = entry; > + continue; > + } > + nir_deref_compare_result comp = nir_compare_deref_paths(dst_path, > entry->path); > + if (comp & nir_derefs_a_contains_b_bit) { > + entry->mask &= ~mask; > + if (entry->mask == 0) { > + nir_instr_remove(&entry->intrin->instr); > + entry->intrin = NULL; > + empty_entry = entry; > + progress = true; > + } > + } > + } > + > + /* Add the new write to the unused array. */ > + struct write_entry new_entry = { > + .intrin = intrin, > + .mask = mask, > + .path = dst_path, > + }; > + > + if (empty_entry) > + *empty_entry = new_entry; > + else > + util_dynarray_append(unused_writes, struct write_entry, new_entry); > + > + return progress; > +} > + > +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. > + 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. > + clear_unused_for_modes(&unused_writes, modes); > + break; > + } > + > + case nir_intrinsic_emit_vertex: > + case nir_intrinsic_emit_vertex_with_counter: { > + nir_variable_mode modes = nir_var_shader_out; > + clear_unused_for_modes(&unused_writes, modes); > Just pass nir_var_shader_out in here; no reason to make a temporary variable. > + break; > + } > + > + case nir_intrinsic_load_deref: { > + nir_deref_instr *src = nir_src_as_deref(intrin->src[0]); > + nir_deref_path *src_path = get_path(state, src); > + > + clear_unused_for_src(&unused_writes, src_path); > + break; > + } > + > + case nir_intrinsic_store_deref: { > + nir_deref_instr *dst = nir_src_as_deref(intrin->src[0]); > + nir_deref_path *dst_path = get_path(state, dst); > + > + uintptr_t mask = nir_intrinsic_write_mask(intrin); > + progress |= update_unused_writes_with_dst(&unused_writes, > intrin, dst_path, mask); > + break; > + } > + > + case nir_intrinsic_copy_deref: { > + nir_deref_instr *src = nir_src_as_deref(intrin->src[1]); > + nir_deref_path *src_path = get_path(state, src); > + > + nir_deref_instr *dst = nir_src_as_deref(intrin->src[0]); > + nir_deref_path *dst_path = get_path(state, dst); > + > + /* Self-copy is removed. */ > + if (src == dst || (nir_compare_deref_paths(src_path, dst_path) & > nir_derefs_equal_bit)) { > + nir_instr_remove(instr); > + progress = true; > + break; > + } > + > + uintptr_t mask = ~(1 << NIR_MAX_VEC_COMPONENTS); > + clear_unused_for_src(&unused_writes, src_path); > + progress |= update_unused_writes_with_dst(&unused_writes, > intrin, dst_path, mask); > + break; > + } > + > + default: > + break; > + } > + } > + > + /* All unused writes at the end of the block are kept, since we can't > be > + * sure they'll be overwritten or not with local analysis only. > + */ > + > + return progress; > +} > + > +static bool > +remove_dead_write_vars_impl(void *mem_ctx, nir_function_impl *func) > +{ > + bool progress = false; > + > + struct state state = { > + .mem_ctx = mem_ctx, > + > + .path_lin_ctx = linear_alloc_parent(mem_ctx, 0), > + .paths = _mesa_hash_table_create(mem_ctx, _mesa_hash_pointer, > _mesa_key_pointer_equal), > + }; > + > + nir_foreach_block(block, func) > + progress |= remove_dead_write_vars_local(&state, block); > + > + return progress; > +} > + > +bool > +nir_opt_dead_write_vars(nir_shader *shader) > +{ > + void *mem_ctx = ralloc_context(NULL); > + bool progress = false; > + > + nir_foreach_function(function, shader) { > + if (!function->impl) > + continue; > + > + nir_metadata_require(function->impl, nir_metadata_block_index); > + > + if (remove_dead_write_vars_impl(mem_ctx, function->impl)) { > + nir_metadata_preserve(function->impl, nir_metadata_block_index | > + nir_metadata_dominance); > I'd put the metadata stuff in the _impl version in case we ever want to expose it for use directly. > + progress = true; > + } > + } > + > + ralloc_free(mem_ctx); > + return progress; > +} > -- > 2.18.0 > > _______________________________________________ > 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