[Mesa-dev] [PATCH 5/9] nir: Add a local dead write vars removal pass
Jason Ekstrand
jason at jlekstrand.net
Wed Aug 22 19:43:38 UTC 2018
On Wed, Aug 15, 2018 at 4:57 PM Caio Marcelo de Oliveira Filho <
caio.oliveira at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180822/cce4e6a6/attachment-0001.html>
More information about the mesa-dev
mailing list