Re: [Mesa-dev] [PATCH 5/9] nir: Add a local dead write vars removal pass
On Wed, Aug 29, 2018 at 3:19 PM Caio Marcelo de Oliveira Filho <
caio.olive...@intel.com> wrote:
> Jason Ekstrand writes:
>
> >> >> +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.
> >>
> >> Makes sense. But I don't get locals are affect? Is this to cover the
> >> parameters being passed to the call?
> >>
> >
> > Because a deref to a local might be passed in as a parameter. This is
> the
> > way pass-by-reference works for SPIR-V.
>
> Will the parameter appear to the new function as local too? If so, will
> they be tagged in a way I can identify the derefs?
>
They'll appear as writes to casts whose source is a load_param intrinsic.
> I'm thinking about what to do with unused writes for locals at the end
> of a function. If it's the main function, we can just remove them, but
> depending on the answer of the question above, it is not so clear for
> non-main functions.
>
Yeah... That's a bit sticky and I'm not sure what the right answer is. I
guess we could, in theory, have a new "param" mode which is sort-of like a
local only it crosses function boundaries. I haven't given that much
though. However, given that functions are always inlined right now, that's
not something we need to deal with just yet.
--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 5/9] nir: Add a local dead write vars removal pass
Jason Ekstrand writes:
>> >> +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.
>>
>> Makes sense. But I don't get locals are affect? Is this to cover the
>> parameters being passed to the call?
>>
>
> Because a deref to a local might be passed in as a parameter. This is the
> way pass-by-reference works for SPIR-V.
Will the parameter appear to the new function as local too? If so, will
they be tagged in a way I can identify the derefs?
I'm thinking about what to do with unused writes for locals at the end
of a function. If it's the main function, we can just remove them, but
depending on the answer of the question above, it is not so clear for
non-main functions.
Thanks,
Caio
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 5/9] nir: Add a local dead write vars removal pass
On Mon, Aug 27, 2018 at 4:55 PM Caio Marcelo de Oliveira Filho <
caio.olive...@intel.com> wrote:
> (Disregard the incomplete mail, still adapting to notmuch-emacs).
>
> Jason Ekstrand writes:
>
> >> +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.
>
> Storing those makes more sense if you read the next commit (the
> "global"). Since I've created the "local" commit as an aid for
> reviewing (perhaps a failure), I did not wanted to change it too much.
>
> When I wrote there were some savings when measuring executions with
> perf.
>
> (...)
>
> >> +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.
>
> Makes sense. But I don't get locals are affect? Is this to cover the
> parameters being passed to the call?
>
Because a deref to a local might be passed in as a parameter. This is the
way pass-by-reference works for SPIR-V.
> >
> >> + 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.
>
> This came from previous code, but except for "system values" it seems to
> do the right thing (note the ~). Is the suggestion to use a "positive"
> enumeration, e.g.
>
Drp... I completely missed the ~. Ignore my comment. Yeah, we could throw
in system values. :D
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 5/9] nir: Add a local dead write vars removal pass
(Disregard the incomplete mail, still adapting to notmuch-emacs).
Jason Ekstrand writes:
>> +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.
Storing those makes more sense if you read the next commit (the
"global"). Since I've created the "local" commit as an aid for
reviewing (perhaps a failure), I did not wanted to change it too much.
When I wrote there were some savings when measuring executions with
perf.
(...)
>> +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.
Makes sense. But I don't get locals are affect? Is this to cover the
parameters being passed to the call?
>
>> + 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.
This came from previous code, but except for "system values" it seems to
do the right thing (note the ~). Is the suggestion to use a "positive"
enumeration, e.g.
clear_unused_for_modes(&unused_writes, nir_var_shader_out |
nir_var_shader_storage |
nir_var_shared);
Thanks,
Caio
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 5/9] nir: Add a local dead write vars removal pass
Jason Ekstrand 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 000..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 e
Re: [Mesa-dev] [PATCH 5/9] nir: Add a local dead write vars removal pass
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 000..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(
