[Mesa-dev] [PATCH 6/8] nir/spirv: Add support for SPV_KHR_variable_pointers

[Jason Ekstrand]

On Fri, Jul 14, 2017 at 2:57 AM, Iago Toral <itoral at igalia.com> wrote:

> On Thu, 2017-07-13 at 12:41 -0700, Jason Ekstrand wrote:
> > ---
> >  src/compiler/spirv/nir_spirv.h     |  1 +
> >  src/compiler/spirv/spirv_to_nir.c  | 42
> > ++++++++++++++++++++++++++++++++---
> >  src/compiler/spirv/vtn_cfg.c       |  5 +++--
> >  src/compiler/spirv/vtn_private.h   | 23 +++++++++++++++++--
> >  src/compiler/spirv/vtn_variables.c | 45
> > +++++++++++++++++++++++++++++++++++---
> >  5 files changed, 106 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/compiler/spirv/nir_spirv.h
> > b/src/compiler/spirv/nir_spirv.h
> > index 7f16866..83577fb 100644
> > --- a/src/compiler/spirv/nir_spirv.h
> > +++ b/src/compiler/spirv/nir_spirv.h
> > @@ -51,6 +51,7 @@ struct nir_spirv_supported_extensions {
> >     bool image_write_without_format;
> >     bool int64;
> >     bool multiview;
> > +   bool variable_pointers;
> >  };
> >
> >  nir_function *spirv_to_nir(const uint32_t *words, size_t word_count,
> > diff --git a/src/compiler/spirv/spirv_to_nir.c
> > b/src/compiler/spirv/spirv_to_nir.c
> > index 6e35f83..e60c9f7 100644
> > --- a/src/compiler/spirv/spirv_to_nir.c
> > +++ b/src/compiler/spirv/spirv_to_nir.c
> > @@ -185,6 +185,13 @@ vtn_ssa_value(struct vtn_builder *b, uint32_t
> > value_id)
> >     case vtn_value_type_ssa:
> >        return val->ssa;
> >
> > +   case vtn_value_type_pointer:
> > +      assert(val->pointer->ptr_type && val->pointer->ptr_type-
> > >type);
> > +      struct vtn_ssa_value *ssa =
> > +         vtn_create_ssa_value(b, val->pointer->ptr_type->type);
> > +      ssa->def = vtn_pointer_to_ssa(b, val->pointer);
> > +      return ssa;
> > +
> >     default:
> >        unreachable("Invalid type for an SSA value");
> >     }
> > @@ -861,9 +868,20 @@ vtn_handle_type(struct vtn_builder *b, SpvOp
> > opcode,
> >           vtn_value(b, w[3], vtn_value_type_type)->type;
> >
> >        val->type->base_type = vtn_base_type_pointer;
> > -      val->type->type = NULL;
>
> Right, we use rzalloc for val->type so this was redundant.
>
> >        val->type->storage_class = storage_class;
> >        val->type->deref = deref_type;
> > +
> > +      struct vtn_type *without_array = deref_type;
> > +      while (without_array->base_type == vtn_base_type_array)
> > +         without_array = without_array->array_element;
> >
>
> What's the purpose of this loop? 'without_array' isn't used for
> anything here...
>

None, aparently.  I had it in there for some reason in an older version but
that reason has aparently been lost.  I'll drop it.


> > +      if (storage_class == SpvStorageClassUniform ||
> > +          storage_class == SpvStorageClassStorageBuffer) {
> > +         /* These can actually be stored to nir_variables and used
> > as SSA
> > +          * values so they need a real glsl_type.
> > +          */
> > +         val->type->type = glsl_vector_type(GLSL_TYPE_UINT, 2);
> > +      }
> >        break;
> >     }
>
> (...)
>
> > static inline struct vtn_value *
> >  vtn_push_value(struct vtn_builder *b, uint32_t value_id,
> >                 enum vtn_value_type value_type)
> > @@ -517,8 +530,14 @@ static inline struct vtn_value *
> >  vtn_push_ssa(struct vtn_builder *b, uint32_t value_id,
> >               struct vtn_type *type, struct vtn_ssa_value *ssa)
> >  {
> > -   struct vtn_value *val = vtn_push_value(b, value_id,
> > vtn_value_type_ssa);
> > -   val->ssa = ssa;
> > +   struct vtn_value *val;
> > +   if (type->base_type == vtn_base_type_pointer) {
> > +      val = vtn_push_value(b, value_id, vtn_value_type_pointer);
> > +      val->pointer = vtn_pointer_from_ssa(b, ssa->def, type);
> > +   } else {
> > +      val = vtn_push_value(b, value_id, vtn_value_type_ssa);
> > +      val->ssa = ssa;
> > +   }
>
> So you start using the 'type' parameter here... I guess it is okay if
> it was added in the previous patch then.
>
> >     return val;
> >  }
> >
> > diff --git a/src/compiler/spirv/vtn_variables.c
> > b/src/compiler/spirv/vtn_variables.c
> > index a9e2dbf..47f111b 100644
> > --- a/src/compiler/spirv/vtn_variables.c
> > +++ b/src/compiler/spirv/vtn_variables.c
> > @@ -223,8 +223,7 @@ vtn_pointer_dereference(struct vtn_builder *b,
> >                          struct vtn_pointer *base,
> >                          struct vtn_access_chain *deref_chain)
> >  {
> > -   if (base->mode == vtn_variable_mode_ubo ||
> > -       base->mode == vtn_variable_mode_ssbo) {
> > +   if (vtn_pointer_uses_ssa_offset(base)) {
> >        return vtn_ssa_offset_pointer_dereference(b, base,
> > deref_chain);
> >     } else {
> >        return vtn_access_chain_pointer_dereference(b, base,
> > deref_chain);
> > @@ -1478,6 +1477,47 @@ vtn_storage_class_to_mode(SpvStorageClass
> > class,
> >     return mode;
> >  }
> >
> > +nir_ssa_def *
> > +vtn_pointer_to_ssa(struct vtn_builder *b, struct vtn_pointer *ptr)
> > +{
> > +   if (ptr->offset && ptr->block_index) {
> > +      return nir_vec2(&b->nb, ptr->block_index, ptr->offset);
> > +   } else {
> > +      /* If we don't have an offset or block index, then we must be
> > a pointer
> > +       * to the variable itself.
> > +       */
> > +      assert(!ptr->offset && !ptr->block_index);
> > +
> > +      /* We can't handle a pointer to an array of descriptors
> > because we have
> > +       * no way of knowing later on that we need to add to update
> > the block
> > +       * index when dereferencing.
> > +       */
> > +      assert(ptr->var && ptr->var->type->base_type ==
> > vtn_base_type_struct);
> > +
> > +      return nir_vec2(&b->nb, vtn_variable_resource_index(b, ptr-
> > >var, NULL),
> > +                              nir_imm_int(&b->nb, 0));
> > +   }
> > +}
> > +
> > +struct vtn_pointer *
> > +vtn_pointer_from_ssa(struct vtn_builder *b, nir_ssa_def *ssa,
> > +                     struct vtn_type *ptr_type)
> > +{
> > +   assert(ssa->num_components == 2 && ssa->bit_size == 32);
> > +   assert(ptr_type->base_type == vtn_base_type_pointer);
> > +   assert(ptr_type->deref->base_type != vtn_base_type_pointer);
> > +
> > +   struct vtn_pointer *ptr = rzalloc(b, struct vtn_pointer);
> > +   ptr->mode = vtn_storage_class_to_mode(ptr_type->storage_class,
> > +                                         ptr_type, NULL);
> > +   ptr->type = ptr_type->deref;
> > +   ptr->ptr_type = ptr_type;
> > +   ptr->block_index = nir_channel(&b->nb, ssa, 0);
> > +   ptr->offset = nir_channel(&b->nb, ssa, 1);
> >
>
> So here we always create (block,offset) pointers, which I guess means
> that we can't call this with ssa values that represent pointers to
> variables


Correct.  Variable pointers only applies to SSBOs.  (Our implementation
also works for UBOs but I don't think the spec requires that.)


> (the else branch in vtn_pointer_to_ssa)... is that checked
> anywhere? It doesn't look like vtn_push_ssa() check anything, so I
> wonder if there is a chance that this can happen.
>

Hrm... Maybe not?  I'm happy to add some type checks.

--Jason


> > +   return ptr;
> > +}
> > +
> >  static bool
> >  is_per_vertex_inout(const struct vtn_variable *var, gl_shader_stage
> > stage)
> >  {
> > @@ -1503,7 +1543,6 @@ vtn_create_variable(struct vtn_builder *b,
> > struct vtn_value *val,
> >  {
> >     assert(ptr_type->base_type == vtn_base_type_pointer);
> >     struct vtn_type *type = ptr_type->deref;
> > -   assert(type->base_type != vtn_base_type_pointer);
> >
> >     struct vtn_type *without_array = type;
> >     while(glsl_type_is_array(without_array->type))
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170714/74f62f16/attachment-0001.html>