<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Mar 15, 2018 at 8:22 AM, Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</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 Thu, Mar 15, 2018 at 6:33 AM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: > This commit adds a new instruction type to NIR for handling derefs. > Nothing uses it yet but this adds the data structure as well as all of > the code to validate, print, clone, and [de]serialize them. > > Cc: Rob Clark <<a href="mailto:robdclark@gmail.com">robdclark@gmail.com</a>> > Cc: Connor Abbott <<a href="mailto:cwabbott0@gmail.com">cwabbott0@gmail.com</a>> > > --- > > This is not tested beyond compile testing. I'm sending it out ahead so > that people can comment on the instruction data structure. I think this > should handle all the SPIR-V use-cases fairly nicely as well as the > use-cases we have today. > > src/compiler/nir/nir.c | 49 +++++++++++++++++++++++ > src/compiler/nir/nir.h | 47 +++++++++++++++++++++- > src/compiler/nir/nir_clone.c | 45 +++++++++++++++++++++ > src/compiler/nir/nir_print.c | 46 ++++++++++++++++++++++ > src/compiler/nir/nir_serialize.c | 85 ++++++++++++++++++++++++++++++++++++++++ > src/compiler/nir/nir_validate.c | 67 +++++++++++++++++++++++++++++++ > 6 files changed, 338 insertions(+), 1 deletion(-) > > diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c > index a97b119..1023eb9 100644 > --- a/src/compiler/nir/nir.c > +++ b/src/compiler/nir/nir.c > @@ -469,6 +469,26 @@ nir_alu_instr_create(nir_shader *shader, nir_op op) > return instr; > } > > +nir_deref_instr * > +nir_deref_instr_create(nir_shader *shader, nir_deref_type deref_type) > +{ > + nir_deref_instr *instr = > + rzalloc_size(shader, sizeof(nir_deref_instr)); > + > + instr_init(&instr->instr, nir_instr_type_deref); > + > + instr->deref_type = deref_type; > + if (deref_type != nir_deref_type_var) > + src_init(&instr->parent); > + > + if (deref_type == nir_deref_type_array_indirect) > + src_init(&instr->arr.indirect); > + > + dest_init(&instr->dest); > + > + return instr; > +} > + > nir_jump_instr * > nir_jump_instr_create(nir_shader *shader, nir_jump_type type) > { > @@ -1198,6 +1218,12 @@ visit_alu_dest(nir_alu_instr *instr, nir_foreach_dest_cb cb, void *state) > } > > static bool > +visit_deref_dest(nir_deref_instr *instr, nir_foreach_dest_cb cb, void *state) > +{ > + return cb(&instr->dest, state); > +} > + > +static bool > visit_intrinsic_dest(nir_intrinsic_instr *instr, nir_foreach_dest_cb cb, > void *state) > { > @@ -1238,6 +1264,8 @@ nir_foreach_dest(nir_instr *instr, nir_foreach_dest_cb cb, void *state) > switch (instr->type) { > case nir_instr_type_alu: > return visit_alu_dest(nir_instr_as_alu(instr), cb, state); > + case nir_instr_type_deref: > + return visit_deref_dest(nir_instr_as_deref(instr), cb, state); > case nir_instr_type_intrinsic: > return visit_intrinsic_dest(nir_instr_as_intrinsic(instr), cb, state); > case nir_instr_type_tex: > @@ -1349,6 +1377,23 @@ visit_alu_src(nir_alu_instr *instr, nir_foreach_src_cb cb, void *state) > } > > static bool > +visit_deref_instr_src(nir_deref_instr *instr, > + nir_foreach_src_cb cb, void *state) > +{ > + if (instr->deref_type != nir_deref_type_var) { > + if (!visit_src(&instr->parent, cb, state)) > + return false; > + } > + > + if (instr->deref_type == nir_deref_type_array_indirect) { > + if (!visit_src(&instr->arr.indirect, cb, state)) > + return false; > + } > + > + return true; > +} > + > +static bool > visit_tex_src(nir_tex_instr *instr, nir_foreach_src_cb cb, void *state) > { > for (unsigned i = 0; i < instr->num_srcs; i++) { > @@ -1436,6 +1481,10 @@ nir_foreach_src(nir_instr *instr, nir_foreach_src_cb cb, void *state) > if (!visit_alu_src(nir_instr_as_alu(instr), cb, state)) > return false; > break; > + case nir_instr_type_deref: > + if (!visit_deref_instr_src(nir_instr_as_deref(instr), cb, state)) > + return false; > + break; > case nir_instr_type_intrinsic: > if (!visit_intrinsic_src(nir_instr_as_intrinsic(instr), cb, state)) > return false; > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > index 839d403..a40a3a0 100644 > --- a/src/compiler/nir/nir.h > +++ b/src/compiler/nir/nir.h > @@ -421,6 +421,7 @@ typedef struct nir_register { > > typedef enum { > nir_instr_type_alu, > + nir_instr_type_deref, > nir_instr_type_call, > nir_instr_type_tex, > nir_instr_type_intrinsic, > @@ -888,7 +889,10 @@ bool nir_alu_srcs_equal(const nir_alu_instr *alu1, const nir_alu_instr *alu2, > typedef enum { > nir_deref_type_var, > nir_deref_type_array, > - nir_deref_type_struct > + nir_deref_type_struct, > + nir_deref_type_array_direct, > + nir_deref_type_array_indirect, > + nir_deref_type_array_wildcard, > } nir_deref_type; > > typedef struct nir_deref { > @@ -950,6 +954,42 @@ nir_deref_tail(nir_deref *deref) > typedef struct { > nir_instr instr; > > + /** The type of this deref instruction */ > + nir_deref_type deref_type; > + > + /** The mode of the underlying variable */ > + nir_variable_mode mode; > + > + /** The dereferenced type of the resulting pointer value */ > + const struct glsl_type *type; > + > + union { > + /** Variable being dereferenced if deref_type is a deref_var */ > + nir_variable *var; > + > + /** Parent deref if deref_type is not deref_var */ > + nir_src parent; > + }; > + > + /** Additional deref parameters */ > + union { > + struct { > + unsigned base_offset; > + nir_src indirect; > + } arr; > + > + struct { > + unsigned index; > + } strct; > + }; > + > + /** Destination to store the resulting "pointer" */ > + nir_dest dest; > +} nir_deref_instr; > + > +typedef struct { > + nir_instr instr; > + > unsigned num_params; > nir_deref_var **params; > nir_deref_var *return_deref; > @@ -1521,6 +1561,8 @@ typedef struct { > > NIR_DEFINE_CAST(nir_instr_as_alu, nir_instr, nir_alu_instr, instr, > type, nir_instr_type_alu) > +NIR_DEFINE_CAST(nir_instr_as_deref, nir_instr, nir_deref_instr, instr, > + type, nir_instr_type_deref) > NIR_DEFINE_CAST(nir_instr_as_call, nir_instr, nir_call_instr, instr, > type, nir_instr_type_call) > NIR_DEFINE_CAST(nir_instr_as_jump, nir_instr, nir_jump_instr, instr, > @@ -2047,6 +2089,9 @@ void nir_metadata_preserve(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(nir_shader *shader, nir_op op); > > +nir_deref_instr *nir_deref_instr_create(nir_shader *shader, > + nir_deref_type deref_type); > + > nir_jump_instr *nir_jump_instr_create(nir_shader *shader, nir_jump_type type); > > nir_load_const_instr *nir_load_const_instr_create(nir_shader *shader, > diff --git a/src/compiler/nir/nir_clone.c b/src/compiler/nir/nir_clone.c > index bcfdaa7..98b3f98 100644 > --- a/src/compiler/nir/nir_clone.c > +++ b/src/compiler/nir/nir_clone.c > @@ -346,6 +346,49 @@ clone_alu(clone_state *state, const nir_alu_instr *alu) > return nalu; > } > > +static nir_deref_instr * > +clone_deref_instr(clone_state *state, const nir_deref_instr *deref) > +{ > + nir_deref_instr *nderef = > + nir_deref_instr_create(state->ns, deref->deref_type); > + > + __clone_dst(state, &nderef->instr, &nderef->dest, &deref->dest); > + > + nderef->mode = deref->mode; > + nderef->type = deref->type; > + > + if (deref->deref_type == nir_deref_type_var) { > + nderef->var = remap_var(state, deref->var); > + } else { > + __clone_src(state, &nderef->instr, &nderef->parent, &deref->parent); > + } > + > + switch (deref->deref_type) { > + case nir_deref_type_struct: > + nderef->strct.index = deref->strct.index; > + break; > + > + case nir_deref_type_array_indirect: > + __clone_src(state, &nderef->instr, > + &nderef->arr.indirect, &deref->arr.indirect); > + /* fall through */ > + > + case nir_deref_type_array_direct: > + nderef->arr.base_offset = deref->arr.base_offset; > + break; > + > + case nir_deref_type_var: > + case nir_deref_type_array_wildcard: > + /* Nothing to do */ > + break; > + > + default: > + unreachable("Invalid instruction deref type"); > + } > + > + return nderef; > +} > + > static nir_intrinsic_instr * > clone_intrinsic(clone_state *state, const nir_intrinsic_instr *itr) > { > @@ -502,6 +545,8 @@ clone_instr(clone_state *state, const nir_instr *instr) > switch (instr->type) { > case nir_instr_type_alu: > return &clone_alu(state, nir_instr_as_alu(instr))->instr; > + case nir_instr_type_deref: > + return &clone_deref_instr(state, nir_instr_as_deref(instr))->instr; > case nir_instr_type_intrinsic: > return &clone_intrinsic(state, nir_instr_as_intrinsic(instr))->instr; > case nir_instr_type_load_const: > diff --git a/src/compiler/nir/nir_print.c b/src/compiler/nir/nir_print.c > index 7888dbd..32aa3fc 100644 > --- a/src/compiler/nir/nir_print.c > +++ b/src/compiler/nir/nir_print.c > @@ -486,6 +486,48 @@ print_var_decl(nir_variable *var, print_state *state) > } > > static void > +print_deref_instr(nir_deref_instr *instr, print_state *state) > +{ > + FILE *fp = state->fp; > + > + print_dest(&instr->dest, state); > + > + if (instr->deref_type == nir_deref_type_var) { > + fprintf(fp, " = %s", get_var_name(instr->var, state)); > + return; > + } > + > + fprintf(fp, " = &"); > + print_src(&instr->parent, state); > + > + assert(instr->parent.is_ssa); > + nir_deref_instr *parent = > + nir_instr_as_deref(instr->parent.ssa->parent_instr); > + > + switch (instr->deref_type) { > + case nir_deref_type_struct: > + fprintf(fp, "->%s", > + glsl_get_struct_elem_name(parent->type, instr->strct.index)); > + break; > + > + case nir_deref_type_array_direct: > + fprintf(fp, "[%u]", instr->arr.base_offset); > + break; > + > + case nir_deref_type_array_indirect: > + fprintf(fp, "["); > + if (instr->arr.base_offset) > + fprintf(fp, "%u + ", instr->arr.base_offset); > + print_src(&instr->arr.indirect, state); > + fprintf(fp, "]"); > + break; > + > + default: > + unreachable("Invalid deref instruction type"); > + } > +} > + > +static void > print_var(nir_variable *var, print_state *state) > { > FILE *fp = state->fp; > @@ -922,6 +964,10 @@ print_instr(const nir_instr *instr, print_state *state, unsigned tabs) > print_alu_instr(nir_instr_as_alu(instr), state); > break; > > + case nir_instr_type_deref: > + print_deref_instr(nir_instr_as_deref(instr), state); > + break; > + > case nir_instr_type_call: > print_call_instr(nir_instr_as_call(instr), state); > break; > diff --git a/src/compiler/nir/nir_serialize.c b/src/compiler/nir/nir_serialize.c > index 00df49c..87e40d9 100644 > --- a/src/compiler/nir/nir_serialize.c > +++ b/src/compiler/nir/nir_serialize.c > @@ -479,6 +479,85 @@ read_alu(read_ctx *ctx) > } > > static void > +write_deref(write_ctx *ctx, const nir_deref_instr *deref) > +{ > + blob_write_uint32(ctx->blob, deref->deref_type); > + > + blob_write_uint32(ctx->blob, deref->mode); > + encode_type_to_blob(ctx->blob, deref->type); > + > + write_dest(ctx, &deref->dest); > + > + if (deref->deref_type == nir_deref_type_var) { > + write_object(ctx, deref->var); > + return; > + } > + > + write_src(ctx, &deref->parent); > + > + switch (deref->deref_type) { > + case nir_deref_type_struct: > + blob_write_uint32(ctx->blob, deref->strct.index); > + break; > + > + case nir_deref_type_array_indirect: > + write_src(ctx, &deref->arr.indirect); > + /* fall through */ > + case nir_deref_type_array_direct: > + blob_write_uint32(ctx->blob, deref->arr.base_offset); > + break; > + > + case nir_deref_type_array_wildcard: > + /* Nothing to do */ > + break; > + > + default: > + unreachable("Invalid deref type"); > + } > +} > + > +static nir_deref_instr * > +read_deref(read_ctx *ctx) > +{ > + nir_deref_type deref_type = blob_read_uint32(ctx->blob); > + nir_deref_instr *deref = nir_deref_instr_create(ctx->nir, deref_type); > + > + deref->mode = blob_read_uint32(ctx->blob); > + deref->type = decode_type_from_blob(ctx->blob); > + > + read_dest(ctx, &deref->dest, &deref->instr); > + > + if (deref_type == nir_deref_type_var) { > + deref->var = read_object(ctx); > + return deref; > + } > + > + read_src(ctx, &deref->parent, &deref->instr); > + > + switch (deref->deref_type) { > + case nir_deref_type_struct: > + deref->strct.index = blob_read_uint32(ctx->blob); > + break; > + > + case nir_deref_type_array_indirect: > + read_src(ctx, &deref->arr.indirect, &deref->instr); > + /* fall through */ > + case nir_deref_type_array_direct: > + deref->arr.base_offset = blob_read_uint32(ctx->blob); > + break; > + > + case nir_deref_type_array_wildcard: > + /* Nothing to do */ > + break; > + > + default: > + unreachable("Invalid deref type"); > + } > + > + return deref; > +} > + > +static void > write_intrinsic(write_ctx *ctx, const nir_intrinsic_instr *intrin) > { > blob_write_uint32(ctx->blob, intrin->intrinsic); > @@ -803,6 +882,9 @@ write_instr(write_ctx *ctx, const nir_instr *instr) > case nir_instr_type_alu: > write_alu(ctx, nir_instr_as_alu(instr)); > break; > + case nir_instr_type_deref: > + write_deref(ctx, nir_instr_as_deref(instr)); > + break; > case nir_instr_type_intrinsic: > write_intrinsic(ctx, nir_instr_as_intrinsic(instr)); > break; > @@ -840,6 +922,9 @@ read_instr(read_ctx *ctx, nir_block *block) > case nir_instr_type_alu: > instr = &read_alu(ctx)->instr; > break; > + case nir_instr_type_deref: > + instr = &read_deref(ctx)->instr; > + break; > case nir_instr_type_intrinsic: > instr = &read_intrinsic(ctx)->instr; > break; > diff --git a/src/compiler/nir/nir_validate.c b/src/compiler/nir/nir_validate.c > index a49948f..354b32b 100644 > --- a/src/compiler/nir/nir_validate.c > +++ b/src/compiler/nir/nir_validate.c > @@ -397,6 +397,69 @@ validate_alu_instr(nir_alu_instr *instr, validate_state *state) > } > > static void > +validate_deref_instr(nir_deref_instr *instr, validate_state *state) > +{ > + if (instr->deref_type == nir_deref_type_var) { > + /* Variable dereferences are stupid simple. */ > + validate_assert(state, instr->mode == instr->var->data.mode); > + validate_assert(state, instr->type == instr->var->type); > + } else { > + /* We require the parent to be SSA. This may be lifted in the future */ > + validate_assert(state, instr->parent.is_ssa); > + > + /* The parent pointer value must have the same number of components > + * as the destination. > + */ > + validate_src(&instr->parent, state, nir_dest_bit_size(instr->dest), > + nir_dest_num_components(instr->dest)); > + > + nir_instr *parent_instr = instr->parent.ssa->parent_instr; > + > + /* The parent must come from another deref instruction */ > + validate_assert(state, parent_instr->type == nir_instr_type_deref); </div></div>I thought a lot of the value was that we could pipe this through things like phis eventually? I'm fine with not starting that way though. <div><div class="h5"></div></div></blockquote><div> </div><div>Yeah, mostly just so that we start small. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > + > + nir_deref_instr *parent = nir_instr_as_deref(parent_instr); > + > + validate_assert(state, instr->mode == parent->mode); > + > + switch (instr->deref_type) { > + case nir_deref_type_struct: > + validate_assert(state, glsl_type_is_struct(parent->type)); > + validate_assert(state, > + instr->strct.index < glsl_get_length(parent->type)); > + validate_assert(state, instr->type == > + glsl_get_struct_field(parent->type, instr->strct.index)); > + break; > + > + case nir_deref_type_array_direct: > + case nir_deref_type_array_indirect: > + case nir_deref_type_array_wildcard: > + validate_assert(state, glsl_type_is_array(parent->type)); > + validate_assert(state, > + instr->type == glsl_get_array_element(parent->type)); > + > + if (instr->deref_type != nir_deref_type_array_wildcard) { > + validate_assert(state, > + instr->arr.base_offset < glsl_get_length(parent->type)); > + } > + > + if (instr->deref_type == nir_deref_type_array_indirect) > + validate_src(&instr->arr.indirect, state, 32, 1); > + break; > + > + default: > + unreachable("Invalid deref instruction type"); > + } > + } > + > + /* We intentionally don't validate the size of the destination because we > + * want to let other compiler components such as SPIR-V decide how big > + * pointers should be. > + */ > + validate_dest(&instr->dest, state, 0, 0); > +} > + > +static void > validate_deref_chain(nir_deref *deref, nir_variable_mode mode, > validate_state *state) > { > @@ -622,6 +685,10 @@ validate_instr(nir_instr *instr, validate_state *state) > validate_alu_instr(nir_instr_as_alu(instr), state); > break; > > + case nir_instr_type_deref: > + validate_deref_instr(nir_instr_as_deref(instr), state); > + break; > + > case nir_instr_type_call: > validate_call_instr(nir_instr_as_call(instr), state); > break; > -- > 2.5.0.400.gff86faf > </div></div>> _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>