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

[Iago Toral]

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...

> +      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 (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.

> +   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))