Re: [Mesa-dev] [PATCH 05/11] nir: Separate dead write removal into its own pass
Latest version is
Reviewed-by: Jason Ekstrand
On Mon, Oct 15, 2018 at 2:41 PM Jason Ekstrand wrote:
> Cool, thanks!
>
> On Mon, Oct 15, 2018 at 2:38 PM Caio Marcelo de Oliveira Filho <
> caio.olive...@intel.com> wrote:
>
>> > > > > +{
>> > > > > + bool progress = false;
>> > > > > +
>> > > > > + /* Find writes that are unused and can be removed. */
>> > > > > + util_dynarray_foreach_reverse(unused_writes, struct
>> write_entry,
>> > > > > entry) {
>> > > > > + nir_deref_compare_result comp = nir_compare_derefs(dst,
>> > > entry->dst);
>> > > > > + if (comp & nir_derefs_a_contains_b_bit) {
>> > > > >
>> > > >
>> > > > Mind throwing an assert in here:
>> > > >
>> > > > assert((comp & nir_derefs_equal_bit) || mask ==
>> > > ~(nir_component_mask_t)0);
>> > >
>> > > We can assert that. We can have an entry for a copy between arrays a
>> > > and b, and see a store a[1].x that will invalidate the 'x' component
>> > > of the copy.
>> > >
>> >
>> > Do you mean, "we can't assert that"?
>>
>> Correct. I meant "we can't".
>>
>>
>> > I'm trying to think about whether or not the type of per-component
>> > invalidation you're talking about there is valid or not. If we can
>> assume
>> > that all struct copies are split and that all copies are fully qualified
>> > (i.e., they end in a vector or scalar with wildcards for all the
>> arrays),
>> > then I think such inference is fine. Maybe worth a comment that such is
>> > intentional?
>>
>> I've added the following comment and assert to update_unused_writes()
>>
>>/* This pass assumes that destination of copies and stores are derefs
>> that
>> * end in a vector or scalar (it is OK to have wildcards or indirects
>> for
>> * arrays).
>> */
>>assert(glsl_type_is_vector_or_scalar(dst->type));
>>
>> My understanding is that in this context this always is true, but in
>> the future might not be if we do things like: "copy a b" instead of
>> "copy a[*] b[*]" when a and b are arrays (similar to structs).
>>
>> Updated my branch with that too.
>>
>>
>>
>> Caio
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/11] nir: Separate dead write removal into its own pass
Cool, thanks!
On Mon, Oct 15, 2018 at 2:38 PM Caio Marcelo de Oliveira Filho <
caio.olive...@intel.com> wrote:
> > > > > +{
> > > > > + bool progress = false;
> > > > > +
> > > > > + /* Find writes that are unused and can be removed. */
> > > > > + util_dynarray_foreach_reverse(unused_writes, struct
> write_entry,
> > > > > entry) {
> > > > > + nir_deref_compare_result comp = nir_compare_derefs(dst,
> > > entry->dst);
> > > > > + if (comp & nir_derefs_a_contains_b_bit) {
> > > > >
> > > >
> > > > Mind throwing an assert in here:
> > > >
> > > > assert((comp & nir_derefs_equal_bit) || mask ==
> > > ~(nir_component_mask_t)0);
> > >
> > > We can assert that. We can have an entry for a copy between arrays a
> > > and b, and see a store a[1].x that will invalidate the 'x' component
> > > of the copy.
> > >
> >
> > Do you mean, "we can't assert that"?
>
> Correct. I meant "we can't".
>
>
> > I'm trying to think about whether or not the type of per-component
> > invalidation you're talking about there is valid or not. If we can
> assume
> > that all struct copies are split and that all copies are fully qualified
> > (i.e., they end in a vector or scalar with wildcards for all the arrays),
> > then I think such inference is fine. Maybe worth a comment that such is
> > intentional?
>
> I've added the following comment and assert to update_unused_writes()
>
>/* This pass assumes that destination of copies and stores are derefs
> that
> * end in a vector or scalar (it is OK to have wildcards or indirects
> for
> * arrays).
> */
>assert(glsl_type_is_vector_or_scalar(dst->type));
>
> My understanding is that in this context this always is true, but in
> the future might not be if we do things like: "copy a b" instead of
> "copy a[*] b[*]" when a and b are arrays (similar to structs).
>
> Updated my branch with that too.
>
>
>
> Caio
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/11] nir: Separate dead write removal into its own pass
> > > > +{
> > > > + bool progress = false;
> > > > +
> > > > + /* Find writes that are unused and can be removed. */
> > > > + util_dynarray_foreach_reverse(unused_writes, struct write_entry,
> > > > entry) {
> > > > + nir_deref_compare_result comp = nir_compare_derefs(dst,
> > entry->dst);
> > > > + if (comp & nir_derefs_a_contains_b_bit) {
> > > >
> > >
> > > Mind throwing an assert in here:
> > >
> > > assert((comp & nir_derefs_equal_bit) || mask ==
> > ~(nir_component_mask_t)0);
> >
> > We can assert that. We can have an entry for a copy between arrays a
> > and b, and see a store a[1].x that will invalidate the 'x' component
> > of the copy.
> >
>
> Do you mean, "we can't assert that"?
Correct. I meant "we can't".
> I'm trying to think about whether or not the type of per-component
> invalidation you're talking about there is valid or not. If we can assume
> that all struct copies are split and that all copies are fully qualified
> (i.e., they end in a vector or scalar with wildcards for all the arrays),
> then I think such inference is fine. Maybe worth a comment that such is
> intentional?
I've added the following comment and assert to update_unused_writes()
/* This pass assumes that destination of copies and stores are derefs that
* end in a vector or scalar (it is OK to have wildcards or indirects for
* arrays).
*/
assert(glsl_type_is_vector_or_scalar(dst->type));
My understanding is that in this context this always is true, but in
the future might not be if we do things like: "copy a b" instead of
"copy a[*] b[*]" when a and b are arrays (similar to structs).
Updated my branch with that too.
Caio
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/11] nir: Separate dead write removal into its own pass
On Mon, Oct 15, 2018 at 12:37 PM Caio Marcelo de Oliveira Filho <
caio.olive...@intel.com> wrote:
> Hi,
>
> > > +{
> > > + bool progress = false;
> > > +
> > > + /* Find writes that are unused and can be removed. */
> > > + util_dynarray_foreach_reverse(unused_writes, struct write_entry,
> > > entry) {
> > > + nir_deref_compare_result comp = nir_compare_derefs(dst,
> entry->dst);
> > > + if (comp & nir_derefs_a_contains_b_bit) {
> > >
> >
> > Mind throwing an assert in here:
> >
> > assert((comp & nir_derefs_equal_bit) || mask ==
> ~(nir_component_mask_t)0);
>
> We can assert that. We can have an entry for a copy between arrays a
> and b, and see a store a[1].x that will invalidate the 'x' component
> of the copy.
>
Do you mean, "we can't assert that"?
I'm trying to think about whether or not the type of per-component
invalidation you're talking about there is valid or not. If we can assume
that all struct copies are split and that all copies are fully qualified
(i.e., they end in a vector or scalar with wildcards for all the arrays),
then I think such inference is fine. Maybe worth a comment that such is
intentional?
(...)
>
> > > + case nir_intrinsic_copy_deref: {
> > > + nir_deref_instr *src = nir_src_as_deref(intrin->src[1]);
> > > + nir_deref_instr *dst = nir_src_as_deref(intrin->src[0]);
> > > +
> > > + /* Self-copy is removed. */
> > > + if (nir_compare_derefs(src, dst) & nir_derefs_equal_bit) {
> > > +nir_instr_remove(instr);
> > > +progress = true;
> > > +break;
> > > + }
> > > +
> > > + uintptr_t mask = ~(1 << NIR_MAX_VEC_COMPONENTS);
> > >
> >
> > I don't think this does quite what you want. Perhaps
> >
> > nir_component_mask_t mask = ~(nir_component_mask_t)0;
>
> I'm going with
>
> nir_component_mask_t mask = (1 << glsl_get_vector_elements(dst->type)) - 1;
>
>
> The idea is that we only fill bits that are valid, so we can detect
> the condition that no bits are set and remove the entry. Sounds good?
>
Seems reasonable. Again, this assumes that dst-type is a vector or scalar
and not a struct, array, or other odd type.
--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/11] nir: Separate dead write removal into its own pass
Hi,
> > +{
> > + bool progress = false;
> > +
> > + /* Find writes that are unused and can be removed. */
> > + util_dynarray_foreach_reverse(unused_writes, struct write_entry,
> > entry) {
> > + nir_deref_compare_result comp = nir_compare_derefs(dst, entry->dst);
> > + if (comp & nir_derefs_a_contains_b_bit) {
> >
>
> Mind throwing an assert in here:
>
> assert((comp & nir_derefs_equal_bit) || mask == ~(nir_component_mask_t)0);
We can assert that. We can have an entry for a copy between arrays a
and b, and see a store a[1].x that will invalidate the 'x' component
of the copy.
(...)
> > + case nir_intrinsic_copy_deref: {
> > + nir_deref_instr *src = nir_src_as_deref(intrin->src[1]);
> > + nir_deref_instr *dst = nir_src_as_deref(intrin->src[0]);
> > +
> > + /* Self-copy is removed. */
> > + if (nir_compare_derefs(src, dst) & nir_derefs_equal_bit) {
> > +nir_instr_remove(instr);
> > +progress = true;
> > +break;
> > + }
> > +
> > + uintptr_t mask = ~(1 << NIR_MAX_VEC_COMPONENTS);
> >
>
> I don't think this does quite what you want. Perhaps
>
> nir_component_mask_t mask = ~(nir_component_mask_t)0;
I'm going with
nir_component_mask_t mask = (1 << glsl_get_vector_elements(dst->type)) - 1;
The idea is that we only fill bits that are valid, so we can detect
the condition that no bits are set and remove the entry. Sounds good?
>
> All of the comments were fairly trivial and nit-picky. Assuming you're ok
> with the changes,
>
> Reviewed-by: Jason Ekstrand
Caio
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/11] nir: Separate dead write removal into its own pass
On Sat, Sep 15, 2018 at 12:46 AM Caio Marcelo de Oliveira Filho <
caio.olive...@intel.com> wrote:
> Instead of doing this as part of the existing copy_prop_vars pass.
>
> Separation makes easier to expand the scope of both passes to be more
> than per-block. For copy propagation, the information about valid
> copies comes from previous instructions; while the dead write removal
> depends on information from later instructions ("have any instruction
> used this deref before overwrite it?").
>
> Also change the tests to use this pass (instead of copy prop vars).
> Note that the disabled tests continue to fail, since the standalone
> pass is still per-block.
>
> v2: Remove entries from dynarray instead of marking items as
> deleted. Use foreach_reverse. (Caio)
>
> (all from Jason)
> Do not cache nir_deref_path. Not worthy for this patch.
> Clear unused writes when hitting a call instruction.
> Clean up enumeration of modes for barriers.
> Move metadata calls to the inner function.
> ---
> 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 | 216 +
> src/compiler/nir/tests/vars_tests.cpp | 3 -
> 5 files changed, 220 insertions(+), 3 deletions(-)
> 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 d3b06564832..b65bb9b80b9 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_find_array_copies.c \
> nir/nir_opt_gcm.c \
> nir/nir_opt_global_to_local.c \
> diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build
> index 1a7fa2d3327..d8f65640004 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_find_array_copies.c',
>'nir_opt_gcm.c',
>'nir_opt_global_to_local.c',
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index 599f469a714..80d145cac1e 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -3030,6 +3030,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_find_array_copies(nir_shader *shader);
>
> bool nir_opt_gcm(nir_shader *shader, bool value_number);
> 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 000..5a3145875cb
> --- /dev/null
> +++ b/src/compiler/nir/nir_opt_dead_write_vars.c
> @@ -0,0 +1,216 @@
> +/*
> + * 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"
> +
> +/**
> + * Elimination of dead writes based on derefs.
> + *
> + * Dead writes are stores and copies that write to a deref, which then
> gets
> + * another write before it was used (read or sourced for a copy). Those
> + * writes can be removed since they don't affect anything.
> + *
> + * For derefs that refer to a memory area that can be read after the
> program,
> + * the last write is considered used. The presence of certain
> instructions
> + * may also cause writes to be considered used, e.g. memory barrier (in
> this case
> + * the value must be written as other thread might
[Mesa-dev] [PATCH 05/11] nir: Separate dead write removal into its own pass
Instead of doing this as part of the existing copy_prop_vars pass.
Separation makes easier to expand the scope of both passes to be more
than per-block. For copy propagation, the information about valid
copies comes from previous instructions; while the dead write removal
depends on information from later instructions ("have any instruction
used this deref before overwrite it?").
Also change the tests to use this pass (instead of copy prop vars).
Note that the disabled tests continue to fail, since the standalone
pass is still per-block.
v2: Remove entries from dynarray instead of marking items as
deleted. Use foreach_reverse. (Caio)
(all from Jason)
Do not cache nir_deref_path. Not worthy for this patch.
Clear unused writes when hitting a call instruction.
Clean up enumeration of modes for barriers.
Move metadata calls to the inner function.
---
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 | 216 +
src/compiler/nir/tests/vars_tests.cpp | 3 -
5 files changed, 220 insertions(+), 3 deletions(-)
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 d3b06564832..b65bb9b80b9 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_find_array_copies.c \
nir/nir_opt_gcm.c \
nir/nir_opt_global_to_local.c \
diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build
index 1a7fa2d3327..d8f65640004 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_find_array_copies.c',
'nir_opt_gcm.c',
'nir_opt_global_to_local.c',
diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index 599f469a714..80d145cac1e 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -3030,6 +3030,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_find_array_copies(nir_shader *shader);
bool nir_opt_gcm(nir_shader *shader, bool value_number);
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 000..5a3145875cb
--- /dev/null
+++ b/src/compiler/nir/nir_opt_dead_write_vars.c
@@ -0,0 +1,216 @@
+/*
+ * 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"
+
+/**
+ * Elimination of dead writes based on derefs.
+ *
+ * Dead writes are stores and copies that write to a deref, which then gets
+ * another write before it was used (read or sourced for a copy). Those
+ * writes can be removed since they don't affect anything.
+ *
+ * For derefs that refer to a memory area that can be read after the program,
+ * the last write is considered used. The presence of certain instructions
+ * may also cause writes to be considered used, e.g. memory barrier (in this
case
+ * the value must be written as other thread might use it).
+ *
+ * The write mask for store instructions is considered, so it is possible that
+ * a store is removed because of the combination of other stores overwritten
+ * its value.
+ */
+
+/* Entry for unused_writes arrays. */
+struct write_entry {
+ /* If NULL indicates the entry is free to be reused. */
+ nir_intrinsic_instr
