[Mesa-dev] [PATCH 045/133] nir: Add a basic metadata management system
Jason Ekstrand
jason at jlekstrand.net
Wed Dec 17 14:59:11 PST 2014
On Wed, Dec 17, 2014 at 11:51 AM, Connor Abbott <cwabbott0 at 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...
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.
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 <jason at 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 (jason at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141217/3e07a0e9/attachment-0001.html>
More information about the mesa-dev
mailing list