On Wed, Dec 17, 2014 at 7:04 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
>
>
> On Tue, Dec 16, 2014 at 10:58 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
>>
>> 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;
>>
>> Bikeshed: I'm a little concerned here about using an enum to represent
>> a bit flag. An enum is supposed to represent a value that can equal
>> one of several things, but this isn't what's going on here. I think
>> that more pedantic compilers like Clang (and perhaps GCC with
>> -Wpedantic) might even generate warnings, since things like:
>>
>> nir_metadata_block_index | nir_metadata_dominance
>>
>> are technically undefined as per the C spec because the resulting
>> value isn't an element of the enum IIRC (I'm too lazy to verify this,
>> though). So overall it seems pretty sketchy, and you don't get most of
>> the good things that come with enums like better type safety anyways
>> since you're masking together values like they're integers.
>
>
> In ANSI C, enums are just integers with the guarantee that it has enough
> bits to hold the biggest value. You can OR them, add them, or even multiply
> if you wish. I don't know that this is allowed in C++ and *maybe* it's
> changed in C99 or C11, but it is allowed in ANSI C. Having them be enums
> also has the nice little benefit that they generate debug symbols and GDB is
> smart enough to look for bitfield combinations and show you
> "nir_metadata_block_index | nir_metadata_dominance" which is pretty nifty...
Ok fine, but if you're going to do it that way then you should change
nir_intrinsic_info.flags to use an enum too to be consistent. Also,
I'd ask that you use (1 << 0), (1 << 1), etc. which is a little more
readable than hex.
>
>>
>> Also, this is inconsistent with the NIR intrinsic info bitfield, which
>> uses #define's for the reasons outlined above. So if you have a strong
>> argument why we should use an enum here, then we should change that to
>> use an enum too.
>>
>> > +
>> > 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
