Jason Ekstrand <ja...@jlekstrand.net> writes:
> 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
