<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Sat, Sep 15, 2018 at 12:46 AM Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com">caio.oliveira@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Instead of doing this as part of the existing copy_prop_vars pass.<br>
<br>
Separation makes easier to expand the scope of both passes to be more<br>
than per-block. For copy propagation, the information about valid<br>
copies comes from previous instructions; while the dead write removal<br>
depends on information from later instructions ("have any instruction<br>
used this deref before overwrite it?").<br>
<br>
Also change the tests to use this pass (instead of copy prop vars).<br>
Note that the disabled tests continue to fail, since the standalone<br>
pass is still per-block.<br>
<br>
v2: Remove entries from dynarray instead of marking items as<br>
deleted. Use foreach_reverse. (Caio)<br>
<br>
(all from Jason)<br>
Do not cache nir_deref_path. Not worthy for this patch.<br>
Clear unused writes when hitting a call instruction.<br>
Clean up enumeration of modes for barriers.<br>
Move metadata calls to the inner function.<br>
---<br>
src/compiler/Makefile.sources | 1 +<br>
src/compiler/nir/meson.build | 1 +<br>
src/compiler/nir/nir.h | 2 +<br>
src/compiler/nir/nir_opt_dead_write_vars.c | 216 +++++++++++++++++++++<br>
src/compiler/nir/tests/vars_tests.cpp | 3 -<br>
5 files changed, 220 insertions(+), 3 deletions(-)<br>
create mode 100644 src/compiler/nir/nir_opt_dead_write_vars.c<br>
<br>
diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources<br>
index d3b06564832..b65bb9b80b9 100644<br>
--- a/src/compiler/Makefile.sources<br>
+++ b/src/compiler/Makefile.sources<br>
@@ -274,6 +274,7 @@ NIR_FILES = \<br>
nir/nir_opt_cse.c \<br>
nir/nir_opt_dce.c \<br>
nir/nir_opt_dead_cf.c \<br>
+ nir/nir_opt_dead_write_vars.c \<br>
nir/nir_opt_find_array_copies.c \<br>
nir/nir_opt_gcm.c \<br>
nir/nir_opt_global_to_local.c \<br>
diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build<br>
index 1a7fa2d3327..d8f65640004 100644<br>
--- a/src/compiler/nir/meson.build<br>
+++ b/src/compiler/nir/meson.build<br>
@@ -158,6 +158,7 @@ files_libnir = files(<br>
'nir_opt_cse.c',<br>
'nir_opt_dce.c',<br>
'nir_opt_dead_cf.c',<br>
+ 'nir_opt_dead_write_vars.c',<br>
'nir_opt_find_array_copies.c',<br>
'nir_opt_gcm.c',<br>
'nir_opt_global_to_local.c',<br>
diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h<br>
index 599f469a714..80d145cac1e 100644<br>
--- a/src/compiler/nir/nir.h<br>
+++ b/src/compiler/nir/nir.h<br>
@@ -3030,6 +3030,8 @@ bool nir_opt_dce(nir_shader *shader);<br>
<br>
bool nir_opt_dead_cf(nir_shader *shader);<br>
<br>
+bool nir_opt_dead_write_vars(nir_shader *shader);<br>
+<br>
bool nir_opt_find_array_copies(nir_shader *shader);<br>
<br>
bool nir_opt_gcm(nir_shader *shader, bool value_number);<br>
diff --git a/src/compiler/nir/nir_opt_dead_write_vars.c b/src/compiler/nir/nir_opt_dead_write_vars.c<br>
new file mode 100644<br>
index 00000000000..5a3145875cb<br>
--- /dev/null<br>
+++ b/src/compiler/nir/nir_opt_dead_write_vars.c<br>
@@ -0,0 +1,216 @@<br>
+/*<br>
+ * Copyright © 2018 Intel Corporation<br>
+ *<br>
+ * Permission is hereby granted, free of charge, to any person obtaining a<br>
+ * copy of this software and associated documentation files (the "Software"),<br>
+ * to deal in the Software without restriction, including without limitation<br>
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,<br>
+ * and/or sell copies of the Software, and to permit persons to whom the<br>
+ * Software is furnished to do so, subject to the following conditions:<br>
+ *<br>
+ * The above copyright notice and this permission notice (including the next<br>
+ * paragraph) shall be included in all copies or substantial portions of the<br>
+ * Software.<br>
+ *<br>
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR<br>
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,<br>
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL<br>
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER<br>
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING<br>
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS<br>
+ * IN THE SOFTWARE.<br>
+ */<br>
+<br>
+#include "nir.h"<br>
+#include "nir_builder.h"<br>
+#include "nir_deref.h"<br>
+<br>
+#include "util/u_dynarray.h"<br>
+<br>
+/**<br>
+ * Elimination of dead writes based on derefs.<br>
+ *<br>
+ * Dead writes are stores and copies that write to a deref, which then gets<br>
+ * another write before it was used (read or sourced for a copy). Those<br>
+ * writes can be removed since they don't affect anything.<br>
+ *<br>
+ * For derefs that refer to a memory area that can be read after the program,<br>
+ * the last write is considered used. The presence of certain instructions<br>
+ * may also cause writes to be considered used, e.g. memory barrier (in this case<br>
+ * the value must be written as other thread might use it).<br>
+ *<br>
+ * The write mask for store instructions is considered, so it is possible that<br>
+ * a store is removed because of the combination of other stores overwritten<br>
+ * its value.<br>
+ */<br>
+<br>
+/* Entry for unused_writes arrays. */<br>
+struct write_entry {<br>
+ /* If NULL indicates the entry is free to be reused. */<br>
+ nir_intrinsic_instr *intrin;<br>
+ uintptr_t mask;<br></blockquote><div><br></div><div>mask should be a nir_component_mask_t.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ nir_deref_instr *dst;<br>
+};<br>
+<br>
+static void<br>
+clear_unused_for_modes(struct util_dynarray *unused_writes, nir_variable_mode modes)<br>
+{<br>
+ util_dynarray_foreach_reverse(unused_writes, struct write_entry, entry) {<br>
+ nir_variable *var = nir_deref_instr_get_variable(entry->dst);<br>
+ if (var->data.mode & modes)<br>
+ *entry = util_dynarray_pop(unused_writes, struct write_entry);<br>
+ }<br>
+}<br>
+<br>
+static void<br>
+clear_unused_for_src(struct util_dynarray *unused_writes, nir_deref_instr *src)<br></blockquote><div><br></div><div>Mind calling this clear_unused_for_read?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+{<br>
+ util_dynarray_foreach_reverse(unused_writes, struct write_entry, entry) {<br>
+ if (nir_compare_derefs(src, entry->dst) & nir_derefs_may_alias_bit)<br>
+ *entry = util_dynarray_pop(unused_writes, struct write_entry);<br>
+ }<br>
+}<br>
+<br>
+static bool<br>
+update_unused_writes_with_dst(struct util_dynarray *unused_writes,<br></blockquote><div><br></div><div>And maybe just update_unused_writes? That sounds weird but update_unused_writes_for_write sounds weirder.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ nir_intrinsic_instr *intrin,<br>
+ nir_deref_instr *dst, uintptr_t mask)<br></blockquote><div><br></div><div>Mask should be a nir_component_mask_t.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+{<br>
+ bool progress = false;<br>
+<br>
+ /* Find writes that are unused and can be removed. */<br>
+ util_dynarray_foreach_reverse(unused_writes, struct write_entry, entry) {<br>
+ nir_deref_compare_result comp = nir_compare_derefs(dst, entry->dst);<br>
+ if (comp & nir_derefs_a_contains_b_bit) {<br></blockquote><div><br></div><div>Mind throwing an assert in here:</div><div><br></div><div>assert((comp & nir_derefs_equal_bit) || mask == ~(nir_component_mask_t)0);<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ entry->mask &= ~mask;<br>
+ if (entry->mask == 0) {<br>
+ nir_instr_remove(&entry->intrin->instr);<br>
+ *entry = util_dynarray_pop(unused_writes, struct write_entry);<br>
+ progress = true;<br>
+ }<br>
+ }<br>
+ }<br>
+<br>
+ /* Add the new write to the unused array. */<br>
+ struct write_entry new_entry = {<br>
+ .intrin = intrin,<br>
+ .mask = mask,<br>
+ .dst = dst,<br>
+ };<br>
+<br>
+ util_dynarray_append(unused_writes, struct write_entry, new_entry);<br>
+<br>
+ return progress;<br>
+}<br>
+<br>
+static bool<br>
+remove_dead_write_vars_local(void *mem_ctx, nir_block *block)<br>
+{<br>
+ bool progress = false;<br>
+<br>
+ struct util_dynarray unused_writes;<br>
+ util_dynarray_init(&unused_writes, mem_ctx);<br>
+<br>
+ nir_foreach_instr_safe(instr, block) {<br>
+ if (instr->type == nir_instr_type_call) {<br>
+ clear_unused_for_modes(&unused_writes, nir_var_shader_out |<br>
+ nir_var_global |<br>
+ nir_var_shader_storage |<br>
+ nir_var_shared);<br></blockquote><div><br></div><div>Also local. We may pass a pointer to a local variable into a function. That's a thing in SPIR-V.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ continue;<br>
+ }<br>
+<br>
+ if (instr->type != nir_instr_type_intrinsic)<br>
+ continue;<br>
+<br>
+ nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);<br>
+ switch (intrin->intrinsic) {<br>
+ case nir_intrinsic_barrier:<br>
+ case nir_intrinsic_memory_barrier: {<br>
+ clear_unused_for_modes(&unused_writes, nir_var_shader_out |<br>
+ nir_var_shader_storage |<br>
+ nir_var_shared);<br>
+ break;<br>
+ }<br>
+<br>
+ case nir_intrinsic_emit_vertex:<br>
+ case nir_intrinsic_emit_vertex_with_counter: {<br>
+ clear_unused_for_modes(&unused_writes, nir_var_shader_out);<br>
+ break;<br>
+ }<br>
+<br>
+ case nir_intrinsic_load_deref: {<br>
+ nir_deref_instr *src = nir_src_as_deref(intrin->src[0]);<br>
+ clear_unused_for_src(&unused_writes, src);<br>
+ break;<br>
+ }<br>
+<br>
+ case nir_intrinsic_store_deref: {<br>
+ nir_deref_instr *dst = nir_src_as_deref(intrin->src[0]);<br>
+ uintptr_t mask = nir_intrinsic_write_mask(intrin);<br></blockquote><div><br></div><div>nir_component_mask_t.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ progress |= update_unused_writes_with_dst(&unused_writes, intrin, dst, mask);<br>
+ break;<br>
+ }<br>
+<br>
+ case nir_intrinsic_copy_deref: {<br>
+ nir_deref_instr *src = nir_src_as_deref(intrin->src[1]);<br>
+ nir_deref_instr *dst = nir_src_as_deref(intrin->src[0]);<br>
+<br>
+ /* Self-copy is removed. */<br>
+ if (nir_compare_derefs(src, dst) & nir_derefs_equal_bit) {<br>
+ nir_instr_remove(instr);<br>
+ progress = true;<br>
+ break;<br>
+ }<br>
+<br>
+ uintptr_t mask = ~(1 << NIR_MAX_VEC_COMPONENTS);<br></blockquote><div><br></div><div>I don't think this does quite what you want. Perhaps<br></div><div><br></div><div>nir_component_mask_t mask = ~(nir_component_mask_t)0;</div><div><br></div><div>All of the comments were fairly trivial and nit-picky. Assuming you're ok with the changes,</div><div><br></div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ clear_unused_for_src(&unused_writes, src);<br>
+ progress |= update_unused_writes_with_dst(&unused_writes, intrin, dst, mask);<br>
+ break;<br>
+ }<br>
+<br>
+ default:<br>
+ break;<br>
+ }<br>
+ }<br>
+<br>
+ /* All unused writes at the end of the block are kept, since we can't be<br>
+ * sure they'll be overwritten or not with local analysis only.<br>
+ */<br>
+<br>
+ return progress;<br>
+}<br>
+<br>
+static bool<br>
+remove_dead_write_vars_impl(void *mem_ctx, nir_function_impl *impl)<br>
+{<br>
+ bool progress = false;<br>
+<br>
+ nir_metadata_require(impl, nir_metadata_block_index);<br>
+<br>
+ nir_foreach_block(block, impl)<br>
+ progress |= remove_dead_write_vars_local(mem_ctx, block);<br>
+<br>
+ if (progress) {<br>
+ nir_metadata_preserve(impl, nir_metadata_block_index |<br>
+ nir_metadata_dominance);<br>
+ }<br>
+<br>
+ return progress;<br>
+}<br>
+<br>
+bool<br>
+nir_opt_dead_write_vars(nir_shader *shader)<br>
+{<br>
+ void *mem_ctx = ralloc_context(NULL);<br>
+ bool progress = false;<br>
+<br>
+ nir_foreach_function(function, shader) {<br>
+ if (!function->impl)<br>
+ continue;<br>
+ progress |= remove_dead_write_vars_impl(mem_ctx, function->impl);<br>
+ }<br>
+<br>
+ ralloc_free(mem_ctx);<br>
+ return progress;<br>
+}<br>
diff --git a/src/compiler/nir/tests/vars_tests.cpp b/src/compiler/nir/tests/vars_tests.cpp<br>
index dd913f04429..cdd2a17fe92 100644<br>
--- a/src/compiler/nir/tests/vars_tests.cpp<br>
+++ b/src/compiler/nir/tests/vars_tests.cpp<br>
@@ -26,9 +26,6 @@<br>
#include "nir.h"<br>
#include "nir_builder.h"<br>
<br>
-/* This optimization is done together with copy propagation. */<br>
-#define nir_opt_dead_write_vars nir_opt_copy_prop_vars<br>
-<br>
namespace {<br>
<br>
class nir_vars_test : public ::testing::Test {<br>
-- <br>
2.19.0<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</blockquote></div></div>