<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Dec 16, 2014 at 10:58 PM, Connor Abbott <<a href="mailto:cwabbott0@gmail.com" target="_blank">cwabbott0@gmail.com</a>> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Dec 16, 2014 at 1:04 AM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> 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; </div></div>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. </blockquote><div> </div><div>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... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. <div><div class="h5"> > + > 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 (<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>) > + */ > + > +#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 > </div></div>> _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div></div>