On Thu, Mar 15, 2018 at 7:46 AM, Rob Clark <[email protected]> wrote:
> On Thu, Mar 15, 2018 at 1:33 AM, Jason Ekstrand <[email protected]> > 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 <[email protected]> > > Cc: Connor Abbott <[email protected]> > > > > --- > > > > 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, > > slightly off topic, but when going thru deref-chain stuff earlier, I > kinda wondered if there was a point to having the 'direct' case for > array's. I mean, there is nir_src_as_const_value().. I guess just > needed when you come out of ssa? > It's useful for alias analysis to just be able to look at he deref type. Honestly, I'm not sure how useful so maybe we can drop it. > > + 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; > > maybe just 'nir_ssa_def *parent' directly? I'm not sure the !ssa case > makes sense for deref instructions. (And same for the instruction > dest) > I thought about that. However, if you want to see these in the back-end after out-of-SSA for raw pointiers in CL, you'll want registers. Also, having a pure-SSA source is actually kind-of painful in NIR because things like nir_foreach_src don't work. > > + }; > > + > > + /** Additional deref parameters */ > > + union { > > + struct { > > + unsigned base_offset; > > + nir_src indirect; > > + } arr; > > + > > + struct { > > + unsigned index; > > + } strct; > > + }; > > Any particular reason not to have a parent nir_deref_instr, with > multiple subclasses (ie. > nir_deref_array_instr/nir_deref_var_instr/etc) vs unions? That would > be more in keeping with how the rest of nir is structured, and avoid > having to use names like "strct" :-P > Because all of the casting you have to do with derefs is painful. Honestly, one of the most annoying bits of dealing with NIR derefs is all of the nir_deref_as_foo() casts. They do provide some safety since, once you do the cast, you can't access struct union members of an array deref. I'm not sure; things may change as I try to make other changes. > Otherwise, this is more or less what I had in mind too. > > BR, > -R > > > + > > + /** 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); > > + > > + 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 > > >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
