[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