[Mesa-dev] [RFC] nir: Add a deref instruction type
Jason Ekstrand
jason at jlekstrand.net
Thu Mar 15 15:51:22 UTC 2018
On Thu, Mar 15, 2018 at 7:46 AM, Rob Clark <robdclark at gmail.com> wrote:
> On Thu, Mar 15, 2018 at 1:33 AM, Jason Ekstrand <jason at jlekstrand.net>
> 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 <robdclark at gmail.com>
> > Cc: Connor Abbott <cwabbott0 at gmail.com>
> >
> > ---
> >
> > 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180315/9592e3f7/attachment-0001.html>
More information about the mesa-dev
mailing list