On Wed, Dec 17, 2014 at 5:59 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
>
>
> On Wed, Dec 17, 2014 at 11:51 AM, Connor Abbott <cwabbo...@gmail.com> wrote:
>>
>> One thing I'm a little worried about is that passes might forget to
>> require the right metadata, and they'll just happen to work since the
>> pass before also requires the same metadata and preserves it. I think
>> a good thing to do to combat this is to have a debug mode that dirties
>> *all* the metadata in nir_metadata_preserve(), even the stuff the
>> caller asks us to preserve, and actually goes and destroys it/somehow
>> makes it garbage so that passes that forget to require the right
>> metadata will fail spectacularly. The other, more insidious, issue is
>> passes accidentally preserving metadata that the author didn't realize
>> they invalidated. We could solve this by somehow re-calculating the
>> metadata in nir_metadata_preserve() and comparing it to the old
>> metadata before trashing it, but I'm not as clear on how that would
>> work and it seems like a lot more work so I'm fine with leaving it out
>> for now. I'd like this to happen whenever we enable nir_validate
>> (#ifdef DEBUG?), but I'll leave figuring out how to enable it up to
>> you.
>
>
> Oh, I am well aware...
>
> As far as preserving goes, I really don't care. If you don't ask for
> metadata but try and use it, that's your fault and there's not much we can
> do about it. Eventually, we may end up trashing everything at the beginning
> of each optimization loop and that may help a little bit. More worrying is
> what happens if you don't call nir_metadata_preserve...
Fine, although I think I'm still a little more worried about it than
you are :) At least it's relatively easy to solve (just trash
everything on nir_metadata_preserve()).
>
> I've thought about a couple of possible solutions to this. One would be to
> have some sort of data structure to represent a pass that contains the
> metadata it requires and the metadata it preserves and never call the
> functions directly. The problem here is that requiring/preserving may be
> more interesting than that. For instance, the nir_lower_variables pass
> requires dominance but only if there's actually stuff to lower. Most
> optimization passes only dirty metadata if they actually do work, etc.
I don't think this will be too bad, though. If we did go down this
approach, then we would still have nir_metadata_require() and
nir_metadata_preserve() as a separate function, so passes like
lower_variables could still call them directly when they realize they
can bail early / need to modify stuff and then get re-analyze it. The
runner would only dirty the metadata if the pass reports making
progress, so that isn't a hard problem to solve. This is the approach
that LLVM takes, so it's been proven before.
>
> Another thought that I've had is, when I make an OPT macro or whatever, we
> can have a flag that gets set before each pass and then unset when you call
> nir_metadata_preserve. If you never call nir_metadata_preserve, we
> assert-fail in debug mode and just dirty the universe in release mode. If
> we did this, we would also need a nir_metadata_preserve_all function for
> optimizations to call if they do no work that just unsets the flag.
>
> So yes, it's on my radar.
> --Jason
>
>>
>>
>> On Tue, Dec 16, 2014 at 1:04 AM, Jason Ekstrand <ja...@jlekstrand.net>
>> wrote:
>> > ---
>> > src/glsl/Makefile.sources | 1 +
>> > src/glsl/nir/nir.c | 19 +++++++---------
>> > src/glsl/nir/nir.h | 21 ++++++++++++++++--
>> > src/glsl/nir/nir_dominance.c | 6 ++---
>> > src/glsl/nir/nir_metadata.c | 52
>> > ++++++++++++++++++++++++++++++++++++++++++++
>> > 5 files changed, 82 insertions(+), 17 deletions(-)
>> > create mode 100644 src/glsl/nir/nir_metadata.c
>> >
>> > diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
>> > index d3b17bd..4eb6320 100644
>> > --- a/src/glsl/Makefile.sources
>> > +++ b/src/glsl/Makefile.sources
>> > @@ -25,6 +25,7 @@ NIR_FILES = \
>> > $(GLSL_SRCDIR)/nir/nir_lower_system_values.c \
>> > $(GLSL_SRCDIR)/nir/nir_lower_variables_scalar.c \
>> > $(GLSL_SRCDIR)/nir/nir_lower_vec_to_movs.c \
>> > + $(GLSL_SRCDIR)/nir/nir_metadata.c \
>> > $(GLSL_SRCDIR)/nir/nir_opcodes.c \
>> > $(GLSL_SRCDIR)/nir/nir_opcodes.h \
>> > $(GLSL_SRCDIR)/nir/nir_opt_copy_propagate.c \
>> > diff --git a/src/glsl/nir/nir.c b/src/glsl/nir/nir.c
>> > index 3dd9e5a..0124799 100644
>> > --- a/src/glsl/nir/nir.c
>> > +++ b/src/glsl/nir/nir.c
>> > @@ -106,6 +106,7 @@ nir_function_create(nir_shader *shader, const char
>> > *name)
>> > exec_list_push_tail(&shader->functions, &func->node);
>> > exec_list_make_empty(&func->overload_list);
>> > func->name = name;
>> > + func->shader = shader;
>> >
>> > return func;
>> > }
>> > @@ -243,8 +244,7 @@ nir_function_impl_create(nir_function_overload
>> > *overload)
>> > impl->return_var = NULL;
>> > impl->reg_alloc = 0;
>> > impl->ssa_alloc = 0;
>> > - impl->block_index_dirty = true;
>> > - impl->dominance_dirty = true;
>> > + impl->valid_metadata = nir_metadata_none;
>> >
>> > /* create start & end blocks */
>> > nir_block *start_block = nir_block_create(mem_ctx);
>> > @@ -815,7 +815,7 @@ handle_jump(nir_block *block)
>> > unlink_block_successors(block);
>> >
>> > nir_function_impl *impl = nir_cf_node_get_function(&block->cf_node);
>> > - impl->dominance_dirty = true;
>> > + nir_metadata_dirty(impl, nir_metadata_none);
>> >
>> > if (jump_instr->type == nir_jump_break ||
>> > jump_instr->type == nir_jump_continue) {
>> > @@ -912,7 +912,7 @@ handle_remove_jump(nir_block *block, nir_jump_type
>> > type)
>> > }
>> >
>> > nir_function_impl *impl = nir_cf_node_get_function(&block->cf_node);
>> > - impl->dominance_dirty = true;
>> > + nir_metadata_dirty(impl, nir_metadata_none);
>> > }
>> >
>> > /**
>> > @@ -1019,8 +1019,7 @@ nir_cf_node_insert_after(nir_cf_node *node,
>> > nir_cf_node *after)
>> > }
>> >
>> > nir_function_impl *impl = nir_cf_node_get_function(node);
>> > - impl->block_index_dirty = true;
>> > - impl->dominance_dirty = true;
>> > + nir_metadata_dirty(impl, nir_metadata_none);
>> > }
>> >
>> > void
>> > @@ -1062,8 +1061,7 @@ nir_cf_node_insert_before(nir_cf_node *node,
>> > nir_cf_node *before)
>> > }
>> >
>> > nir_function_impl *impl = nir_cf_node_get_function(node);
>> > - impl->block_index_dirty = true;
>> > - impl->dominance_dirty = true;
>> > + nir_metadata_dirty(impl, nir_metadata_none);
>> > }
>> >
>> > void
>> > @@ -1109,7 +1107,7 @@ void
>> > nir_cf_node_remove(nir_cf_node *node)
>> > {
>> > nir_function_impl *impl = nir_cf_node_get_function(node);
>> > - impl->block_index_dirty = true;
>> > + nir_metadata_dirty(impl, nir_metadata_none);
>> >
>> > if (node->type == nir_cf_node_block) {
>> > /*
>> > @@ -1673,13 +1671,12 @@ nir_index_blocks(nir_function_impl *impl)
>> > {
>> > unsigned index = 0;
>> >
>> > - if (!impl->block_index_dirty)
>> > + if (impl->valid_metadata & nir_metadata_block_index)
>> > return;
>> >
>> > nir_foreach_block(impl, index_block, &index);
>> >
>> > impl->num_blocks = index;
>> > - impl->block_index_dirty = false;
>> > }
>> >
>> > static void
>> > diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
>> > index c9146c0..8d5f6b8 100644
>> > --- a/src/glsl/nir/nir.h
>> > +++ b/src/glsl/nir/nir.h
>> > @@ -33,6 +33,7 @@
>> > #include "GL/gl.h" /* GLenum */
>> > #include "util/ralloc.h"
>> > #include "main/mtypes.h"
>> > +#include "main/bitset.h"
>> > #include "nir_types.h"
>> > #include <stdio.h>
>> >
>> > @@ -45,6 +46,7 @@ extern "C" {
>> >
>> > struct nir_function_overload;
>> > struct nir_function;
>> > +struct nir_shader;
>> >
>> >
>> > /**
>> > @@ -1037,6 +1039,16 @@ typedef struct {
>> > #define nir_loop_last_cf_node(loop) \
>> > exec_node_data(nir_cf_node, exec_list_get_tail(&(loop)->body), node)
>> >
>> > +/**
>> > + * Various bits of metadata that can may be created or required by
>> > + * optimization and analysis passes
>> > + */
>> > +typedef enum {
>> > + nir_metadata_none = 0x0,
>> > + nir_metadata_block_index = 0x1,
>> > + nir_metadata_dominance = 0x2,
>> > +} nir_metadata;
>> > +
>> > typedef struct {
>> > nir_cf_node cf_node;
>> >
>> > @@ -1069,8 +1081,7 @@ typedef struct {
>> > /* total number of basic blocks, only valid when block_index_dirty =
>> > false */
>> > unsigned num_blocks;
>> >
>> > - bool block_index_dirty;
>> > - bool dominance_dirty;
>> > + nir_metadata valid_metadata;
>> > } nir_function_impl;
>> >
>> > #define nir_cf_node_next(_node) \
>> > @@ -1126,6 +1137,7 @@ typedef struct nir_function {
>> >
>> > struct exec_list overload_list;
>> > const char *name;
>> > + struct nir_shader *shader;
>> > } nir_function;
>> >
>> > #define nir_function_first_overload(func) \
>> > @@ -1209,6 +1221,11 @@ void nir_cf_node_insert_end(struct exec_list
>> > *list, nir_cf_node *node);
>> > /** removes a control flow node, doing any cleanup necessary */
>> > void nir_cf_node_remove(nir_cf_node *node);
>> >
>> > +/** requests that the given pieces of metadata be generated */
>> > +void nir_metadata_require(nir_function_impl *impl, nir_metadata
>> > required);
>> > +/** dirties all but the preserved metadata */
>> > +void nir_metadata_dirty(nir_function_impl *impl, nir_metadata
>> > preserved);
>> > +
>> > /** creates an instruction with default swizzle/writemask/etc. with
>> > NULL registers */
>> > nir_alu_instr *nir_alu_instr_create(void *mem_ctx, nir_op op);
>> >
>> > diff --git a/src/glsl/nir/nir_dominance.c b/src/glsl/nir/nir_dominance.c
>> > index 988a3fc..7684784 100644
>> > --- a/src/glsl/nir/nir_dominance.c
>> > +++ b/src/glsl/nir/nir_dominance.c
>> > @@ -181,10 +181,10 @@ calc_dom_children(nir_function_impl* impl)
>> > void
>> > nir_calc_dominance_impl(nir_function_impl *impl)
>> > {
>> > - if (!impl->dominance_dirty)
>> > + if (impl->valid_metadata & nir_metadata_dominance)
>> > return;
>> >
>> > - nir_index_blocks(impl);
>> > + nir_metadata_require(impl, nir_metadata_block_index);
>> >
>> > dom_state state;
>> > state.impl = impl;
>> > @@ -202,8 +202,6 @@ nir_calc_dominance_impl(nir_function_impl *impl)
>> > impl->start_block->imm_dom = NULL;
>> >
>> > calc_dom_children(impl);
>> > -
>> > - impl->dominance_dirty = false;
>> > }
>> >
>> > void
>> > diff --git a/src/glsl/nir/nir_metadata.c b/src/glsl/nir/nir_metadata.c
>> > new file mode 100644
>> > index 0000000..070cb74
>> > --- /dev/null
>> > +++ b/src/glsl/nir/nir_metadata.c
>> > @@ -0,0 +1,52 @@
>> > +/*
>> > + * Copyright © 2014 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.
>> > + *
>> > + * Authors:
>> > + * Jason Ekstrand (ja...@jlekstrand.net)
>> > + */
>> > +
>> > +#include "nir.h"
>> > +
>> > +/*
>> > + * Handles management of the metadata.
>> > + */
>> > +
>> > +void
>> > +nir_metadata_require(nir_function_impl *impl, nir_metadata required)
>> > +{
>> > +#define NEEDS_UPDATE(X) ((required & ~impl->valid_metadata) & (X))
>> > +
>> > + if (NEEDS_UPDATE(nir_metadata_block_index))
>> > + nir_index_blocks(impl);
>> > + if (NEEDS_UPDATE(nir_metadata_dominance))
>> > + nir_calc_dominance_impl(impl);
>> > +
>> > +#undef NEEDS_UPDATE
>> > +
>> > + impl->valid_metadata |= required;
>> > +}
>> > +
>> > +void
>> > +nir_metadata_dirty(nir_function_impl *impl, nir_metadata preserved)
>> > +{
>> > + impl->valid_metadata &= preserved;
>> > +}
>> > --
>> > 2.2.0
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
