[Mesa-dev] [PATCH v3 1/2] spirv/nir: handle memory access qualifiers for SSBO loads/stores

Jason Ekstrand jason at jlekstrand.net
Thu Oct 11 15:15:43 UTC 2018


Ok, I think there's a decent chance we've got them all now.

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

It'd be great if we had some sort of test which put the access qualifiers
on the variable so we could be sure it worked. :/

On Wed, Oct 10, 2018 at 3:43 AM Samuel Pitoiset <samuel.pitoiset at gmail.com>
wrote:

> v2: - change how the access qualifiers are accumulated
> v3: - duplicate members in struct_member_decoration_cb()
>     - handle access qualifiers on variables
>     - remove access qualifiers handling in _vtn_variable_load_store()
>     - fix setting access qualifiers on type->array_element
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
> ---
>  src/compiler/nir/nir_intrinsics.py |  4 +--
>  src/compiler/spirv/spirv_to_nir.c  | 24 +++++++++++--
>  src/compiler/spirv/vtn_private.h   |  9 +++++
>  src/compiler/spirv/vtn_variables.c | 54 +++++++++++++++++++++++++-----
>  4 files changed, 77 insertions(+), 14 deletions(-)
>
> diff --git a/src/compiler/nir/nir_intrinsics.py
> b/src/compiler/nir/nir_intrinsics.py
> index b06b38fc2c..ec3049ca06 100644
> --- a/src/compiler/nir/nir_intrinsics.py
> +++ b/src/compiler/nir/nir_intrinsics.py
> @@ -565,7 +565,7 @@ intrinsic("load_interpolated_input", src_comp=[2, 1],
> dest_comp=0,
>            indices=[BASE, COMPONENT], flags=[CAN_ELIMINATE, CAN_REORDER])
>
>  # src[] = { buffer_index, offset }. No const_index
> -load("ssbo", 2, flags=[CAN_ELIMINATE])
> +load("ssbo", 2, flags=[CAN_ELIMINATE], indices=[ACCESS])
>  # src[] = { offset }. const_index[] = { base, component }
>  load("output", 1, [BASE, COMPONENT], flags=[CAN_ELIMINATE])
>  # src[] = { vertex, offset }. const_index[] = { base }
> @@ -591,6 +591,6 @@ store("output", 2, [BASE, WRMASK, COMPONENT])
>  # const_index[] = { base, write_mask, component }
>  store("per_vertex_output", 3, [BASE, WRMASK, COMPONENT])
>  # src[] = { value, block_index, offset }. const_index[] = { write_mask }
> -store("ssbo", 3, [WRMASK])
> +store("ssbo", 3, [WRMASK, ACCESS])
>  # src[] = { value, offset }. const_index[] = { base, write_mask }
>  store("shared", 2, [BASE, WRMASK])
> diff --git a/src/compiler/spirv/spirv_to_nir.c
> b/src/compiler/spirv/spirv_to_nir.c
> index 2ad83196e4..37a801037b 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -668,6 +668,16 @@ mutable_matrix_member(struct vtn_builder *b, struct
> vtn_type *type, int member)
>     return type;
>  }
>
> +static void
> +vtn_handle_access_qualifier(struct vtn_builder *b, struct vtn_type *type,
> +                            int member, enum gl_access_qualifier access)
> +{
> +   type->members[member] = vtn_type_copy(b, type->members[member]);
> +   type = type->members[member];
> +
> +   type->access |= access;
> +}
> +
>  static void
>  struct_member_decoration_cb(struct vtn_builder *b,
>                              struct vtn_value *val, int member,
> @@ -681,13 +691,21 @@ struct_member_decoration_cb(struct vtn_builder *b,
>     assert(member < ctx->num_fields);
>
>     switch (dec->decoration) {
> +   case SpvDecorationRelaxedPrecision:
> +   case SpvDecorationUniform:
> +      break; /* FIXME: Do nothing with this for now. */
>     case SpvDecorationNonWritable:
> +      vtn_handle_access_qualifier(b, ctx->type, member,
> ACCESS_NON_WRITEABLE);
> +      break;
>     case SpvDecorationNonReadable:
> -   case SpvDecorationRelaxedPrecision:
> +      vtn_handle_access_qualifier(b, ctx->type, member,
> ACCESS_NON_READABLE);
> +      break;
>     case SpvDecorationVolatile:
> +      vtn_handle_access_qualifier(b, ctx->type, member, ACCESS_VOLATILE);
> +      break;
>     case SpvDecorationCoherent:
> -   case SpvDecorationUniform:
> -      break; /* FIXME: Do nothing with this for now. */
> +      vtn_handle_access_qualifier(b, ctx->type, member, ACCESS_COHERENT);
> +      break;
>     case SpvDecorationNoPerspective:
>        ctx->fields[member].interpolation = INTERP_MODE_NOPERSPECTIVE;
>        break;
> diff --git a/src/compiler/spirv/vtn_private.h
> b/src/compiler/spirv/vtn_private.h
> index a31202d129..da7a04ce59 100644
> --- a/src/compiler/spirv/vtn_private.h
> +++ b/src/compiler/spirv/vtn_private.h
> @@ -296,6 +296,9 @@ struct vtn_type {
>     /* for arrays, matrices and pointers, the array stride */
>     unsigned stride;
>
> +   /* Access qualifiers */
> +   enum gl_access_qualifier access;
> +
>     union {
>        /* Members for scalar, vector, and array-like types */
>        struct {
> @@ -457,6 +460,9 @@ struct vtn_pointer {
>     /** A (block_index, offset) pair representing a UBO or SSBO position.
> */
>     struct nir_ssa_def *block_index;
>     struct nir_ssa_def *offset;
> +
> +   /* Access qualifiers */
> +   enum gl_access_qualifier access;
>  };
>
>  struct vtn_variable {
> @@ -488,6 +494,9 @@ struct vtn_variable {
>      * hack at some point in the future.
>      */
>     struct vtn_pointer *copy_prop_sampler;
> +
> +   /* Access qualifiers. */
> +   enum gl_access_qualifier access;
>  };
>
>  struct vtn_image_pointer {
> diff --git a/src/compiler/spirv/vtn_variables.c
> b/src/compiler/spirv/vtn_variables.c
> index 636fdb8689..cc3438bff2 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -89,6 +89,7 @@ vtn_access_chain_pointer_dereference(struct vtn_builder
> *b,
>     struct vtn_access_chain *chain =
>        vtn_access_chain_extend(b, base->chain, deref_chain->length);
>     struct vtn_type *type = base->type;
> +   enum gl_access_qualifier access = base->access;
>
>     /* OpPtrAccessChain is only allowed on things which support variable
>      * pointers.  For everything else, the client is expected to just pass
> us
> @@ -106,6 +107,8 @@ vtn_access_chain_pointer_dereference(struct
> vtn_builder *b,
>        } else {
>           type = type->array_element;
>        }
> +
> +      access |= type->access;
>     }
>
>     struct vtn_pointer *ptr = rzalloc(b, struct vtn_pointer);
> @@ -114,6 +117,7 @@ vtn_access_chain_pointer_dereference(struct
> vtn_builder *b,
>     ptr->var = base->var;
>     ptr->deref = base->deref;
>     ptr->chain = chain;
> +   ptr->access = access;
>
>     return ptr;
>  }
> @@ -184,6 +188,7 @@ vtn_ssa_offset_pointer_dereference(struct vtn_builder
> *b,
>     nir_ssa_def *block_index = base->block_index;
>     nir_ssa_def *offset = base->offset;
>     struct vtn_type *type = base->type;
> +   enum gl_access_qualifier access = base->access;
>
>     unsigned idx = 0;
>     if (base->mode == vtn_variable_mode_ubo ||
> @@ -198,6 +203,7 @@ vtn_ssa_offset_pointer_dereference(struct vtn_builder
> *b,
>                 idx++;
>                 /* This consumes a level of type */
>                 type = type->array_element;
> +               access |= type->access;
>              } else {
>                 /* This is annoying.  We've been asked for a pointer to the
>                  * array of UBOs/SSBOs and not a specifc buffer.  Return a
> @@ -319,6 +325,7 @@ vtn_ssa_offset_pointer_dereference(struct vtn_builder
> *b,
>              vtn_access_link_as_ssa(b, deref_chain->link[idx],
> type->stride);
>           offset = nir_iadd(&b->nb, offset, elem_offset);
>           type = type->array_element;
> +         access |= type->access;
>           break;
>        }
>
> @@ -328,6 +335,7 @@ vtn_ssa_offset_pointer_dereference(struct vtn_builder
> *b,
>           nir_ssa_def *mem_offset = nir_imm_int(&b->nb,
> type->offsets[member]);
>           offset = nir_iadd(&b->nb, offset, mem_offset);
>           type = type->members[member];
> +         access |= type->access;
>           break;
>        }
>
> @@ -341,6 +349,7 @@ vtn_ssa_offset_pointer_dereference(struct vtn_builder
> *b,
>     ptr->type = type;
>     ptr->block_index = block_index;
>     ptr->offset = offset;
> +   ptr->access = access;
>
>     return ptr;
>  }
> @@ -370,6 +379,7 @@ vtn_pointer_for_variable(struct vtn_builder *b,
>     vtn_assert(ptr_type->deref->type == var->type->type);
>     pointer->ptr_type = ptr_type;
>     pointer->var = var;
> +   pointer->access = var->access | var->type->access;
>
>     return pointer;
>  }
> @@ -608,7 +618,8 @@ static void
>  _vtn_load_store_tail(struct vtn_builder *b, nir_intrinsic_op op, bool
> load,
>                       nir_ssa_def *index, nir_ssa_def *offset,
>                       unsigned access_offset, unsigned access_size,
> -                     struct vtn_ssa_value **inout, const struct glsl_type
> *type)
> +                     struct vtn_ssa_value **inout, const struct glsl_type
> *type,
> +                     enum gl_access_qualifier access)
>  {
>     nir_intrinsic_instr *instr = nir_intrinsic_instr_create(b->nb.shader,
> op);
>     instr->num_components = glsl_get_vector_elements(type);
> @@ -624,6 +635,11 @@ _vtn_load_store_tail(struct vtn_builder *b,
> nir_intrinsic_op op, bool load,
>        nir_intrinsic_set_range(instr, access_size);
>     }
>
> +   if (op == nir_intrinsic_load_ssbo ||
> +       op == nir_intrinsic_store_ssbo) {
> +      nir_intrinsic_set_access(instr, access);
> +   }
> +
>     if (index)
>        instr->src[src++] = nir_src_for_ssa(index);
>
> @@ -654,7 +670,8 @@ static void
>  _vtn_block_load_store(struct vtn_builder *b, nir_intrinsic_op op, bool
> load,
>                        nir_ssa_def *index, nir_ssa_def *offset,
>                        unsigned access_offset, unsigned access_size,
> -                      struct vtn_type *type, struct vtn_ssa_value **inout)
> +                      struct vtn_type *type, enum gl_access_qualifier
> access,
> +                      struct vtn_ssa_value **inout)
>  {
>     if (load && *inout == NULL)
>        *inout = vtn_create_ssa_value(b, type->type);
> @@ -704,7 +721,8 @@ _vtn_block_load_store(struct vtn_builder *b,
> nir_intrinsic_op op, bool load,
>              _vtn_load_store_tail(b, op, load, index, elem_offset,
>                                   access_offset, access_size,
>                                   &(*inout)->elems[i],
> -                                 glsl_vector_type(base_type, vec_width));
> +                                 glsl_vector_type(base_type, vec_width),
> +                                 type->access | access);
>           }
>
>           if (load && type->row_major)
> @@ -717,7 +735,8 @@ _vtn_block_load_store(struct vtn_builder *b,
> nir_intrinsic_op op, bool load,
>              vtn_assert(glsl_type_is_vector_or_scalar(type->type));
>              _vtn_load_store_tail(b, op, load, index, offset,
>                                   access_offset, access_size,
> -                                 inout, type->type);
> +                                 inout, type->type,
> +                                 type->access | access);
>           } else {
>              /* This is a strided load.  We have to load N things
> separately.
>               * This is the single column of a row-major matrix case.
> @@ -738,7 +757,8 @@ _vtn_block_load_store(struct vtn_builder *b,
> nir_intrinsic_op op, bool load,
>                 comp = &temp_val;
>                 _vtn_load_store_tail(b, op, load, index, elem_offset,
>                                      access_offset, access_size,
> -                                    &comp, glsl_scalar_type(base_type));
> +                                    &comp, glsl_scalar_type(base_type),
> +                                    type->access | access);
>                 per_comp[i] = comp->def;
>              }
>
> @@ -758,7 +778,9 @@ _vtn_block_load_store(struct vtn_builder *b,
> nir_intrinsic_op op, bool load,
>              nir_iadd(&b->nb, offset, nir_imm_int(&b->nb, i *
> type->stride));
>           _vtn_block_load_store(b, op, load, index, elem_off,
>                                 access_offset, access_size,
> -                               type->array_element, &(*inout)->elems[i]);
> +                               type->array_element,
> +                               type->array_element->access | access,
> +                               &(*inout)->elems[i]);
>        }
>        return;
>     }
> @@ -770,7 +792,9 @@ _vtn_block_load_store(struct vtn_builder *b,
> nir_intrinsic_op op, bool load,
>              nir_iadd(&b->nb, offset, nir_imm_int(&b->nb,
> type->offsets[i]));
>           _vtn_block_load_store(b, op, load, index, elem_off,
>                                 access_offset, access_size,
> -                               type->members[i], &(*inout)->elems[i]);
> +                               type->members[i],
> +                               type->members[i]->access | access,
> +                               &(*inout)->elems[i]);
>        }
>        return;
>     }
> @@ -809,7 +833,7 @@ vtn_block_load(struct vtn_builder *b, struct
> vtn_pointer *src)
>     struct vtn_ssa_value *value = NULL;
>     _vtn_block_load_store(b, op, true, index, offset,
>                           access_offset, access_size,
> -                         src->type, &value);
> +                         src->type, src->access, &value);
>     return value;
>  }
>
> @@ -833,7 +857,7 @@ vtn_block_store(struct vtn_builder *b, struct
> vtn_ssa_value *src,
>     offset = vtn_pointer_to_offset(b, dst, &index);
>
>     _vtn_block_load_store(b, op, false, index, offset,
> -                         0, 0, dst->type, &src);
> +                         0, 0, dst->type, dst->access, &src);
>  }
>
>  static void
> @@ -1389,6 +1413,18 @@ var_decoration_cb(struct vtn_builder *b, struct
> vtn_value *val, int member,
>     case SpvDecorationOffset:
>        vtn_var->offset = dec->literals[0];
>        break;
> +   case SpvDecorationNonWritable:
> +      vtn_var->access |= ACCESS_NON_WRITEABLE;
> +      break;
> +   case SpvDecorationNonReadable:
> +      vtn_var->access |= ACCESS_NON_READABLE;
> +      break;
> +   case SpvDecorationVolatile:
> +      vtn_var->access |= ACCESS_VOLATILE;
> +      break;
> +   case SpvDecorationCoherent:
> +      vtn_var->access |= ACCESS_COHERENT;
> +      break;
>     default:
>        break;
>     }
> --
> 2.19.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181011/61f97a63/attachment-0001.html>


More information about the mesa-dev mailing list