<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 14, 2017 at 1:53 AM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, 2017-07-13 at 12:41 -0700, Jason Ekstrand wrote:<br>
> ---<br>
>  src/compiler/spirv/spirv_to_<wbr>nir.c  |  4 +++-<br>
>  src/compiler/spirv/vtn_<wbr>private.h   | 11 ++++++++---<br>
>  src/compiler/spirv/vtn_<wbr>variables.c | 23 +++++++++++++++++++++++<br>
>  3 files changed, 34 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/src/compiler/spirv/spirv_to_<wbr>nir.c<br>
> b/src/compiler/spirv/spirv_to_<wbr>nir.c<br>
> index 89ebc5f..7038bd9 100644<br>
> --- a/src/compiler/spirv/spirv_to_<wbr>nir.c<br>
> +++ b/src/compiler/spirv/spirv_to_<wbr>nir.c<br>
> @@ -600,7 +600,8 @@ type_decoration_cb(struct vtn_builder *b,<br>
>     switch (dec->decoration) {<br>
>     case SpvDecorationArrayStride:<br>
>        assert(type->base_type == vtn_base_type_matrix ||<br>
> -             type->base_type == vtn_base_type_array);<br>
> +             type->base_type == vtn_base_type_array ||<br>
> +             type->base_type == vtn_base_type_pointer);<br>
>        type->stride = dec->literals[0];<br>
>        break;<br>
>     case SpvDecorationBlock:<br>
> @@ -3067,6 +3068,7 @@ vtn_handle_body_instruction(<wbr>struct vtn_builder<br>
> *b, SpvOp opcode,<br>
>     case SpvOpCopyMemory:<br>
>     case SpvOpCopyMemorySized:<br>
>     case SpvOpAccessChain:<br>
> +   case SpvOpPtrAccessChain:<br>
>     case SpvOpInBoundsAccessChain:<br>
>     case SpvOpArrayLength:<br>
>        vtn_handle_variables(b, opcode, w, count);<br>
> diff --git a/src/compiler/spirv/vtn_<wbr>private.h<br>
> b/src/compiler/spirv/vtn_<wbr>private.h<br>
> index 7cb5035..2f96c09 100644<br>
> --- a/src/compiler/spirv/vtn_<wbr>private.h<br>
> +++ b/src/compiler/spirv/vtn_<wbr>private.h<br>
> @@ -220,15 +220,15 @@ struct vtn_type {<br>
>     /* Specifies the length of complex types. */<br>
>     unsigned length;<br>
>  <br>
> +   /* for arrays, matrices and pointers, the array stride */<br>
> +   unsigned stride;<br>
> +<br>
>     union {<br>
>        /* Members for scalar, vector, and array-like types */<br>
>        struct {<br>
>           /* for arrays, the vtn_type for the elements of the array<br>
> */<br>
>           struct vtn_type *array_element;<br>
>  <br>
> -         /* for arrays and matrices, the array stride */<br>
> -         unsigned stride;<br>
> -<br>
>           /* for matrices, whether the matrix is stored row-major */<br>
>           bool row_major:1;<br>
>  <br>
> @@ -308,6 +308,11 @@ struct vtn_access_link {<br>
>  struct vtn_access_chain {<br>
>     uint32_t length;<br>
>  <br>
> +   /** Whether or not to treat the base pointer as an array.  This<br>
> is only<br>
> +    * true if this access chain came from an OpPtrAccessChain.<br>
> +    */<br>
> +   bool ptr_as_array;<br>
> +<br>
>     /** Struct elements and array offsets.<br>
>      *<br>
>      * This is an array of 1 so that it can conveniently be created<br>
> on the<br>
> diff --git a/src/compiler/spirv/vtn_<wbr>variables.c<br>
> b/src/compiler/spirv/vtn_<wbr>variables.c<br>
> index 4f21fdd..a9ba392 100644<br>
> --- a/src/compiler/spirv/vtn_<wbr>variables.c<br>
> +++ b/src/compiler/spirv/vtn_<wbr>variables.c<br>
> @@ -67,6 +67,12 @@ vtn_access_chain_pointer_<wbr>dereference(struct<br>
> vtn_builder *b,<br>
>        vtn_access_chain_<wbr>extend(b, base->chain, deref_chain->length);<br>
>     struct vtn_type *type = base->type;<br>
>  <br>
> +   /* OpPtrAccessChain is only allowed on things which support<br>
> variable<br>
> +    * pointers.  For everything else, the client is expected to just<br>
> pass us<br>
> +    * the right access chain.<br>
> +    */<br>
> +   assert(!deref_chain->ptr_<wbr>as_array);<br>
> +<br>
>     unsigned start = base->chain ? base->chain->length : 0;<br>
>     for (unsigned i = 0; i < deref_chain->length; i++) {<br>
>        chain->link[start + i] = deref_chain->link[i];<br>
> @@ -135,6 +141,21 @@ vtn_ssa_offset_pointer_<wbr>dereference(struct<br>
> vtn_builder *b,<br>
>     struct vtn_type *type = base->type;<br>
>  <br>
>     unsigned idx = 0;<br>
> +   if (deref_chain->ptr_as_array) {<br>
> +      /* We need ptr_type for the stride */<br>
> +      assert(base->ptr_type);<br>
> +      /* This must be a pointer to an actual element somewhere */<br>
> +      assert(block_index && offset);<br>
> +      /* We need at least one element in the chain */<br>
> +      assert(deref_chain-><wbr>length >= 1);<br>
<br>
</div></div>I was surprised that this wasn't '>= 2'</blockquote><div><br></div><div>A chain with length 0 (or null chain) means just the variable itself.  I suppose we could handle length == 0 as a no-op if we wanted.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> but looking at the<br>
implementation in vtn_handle_variables() it seems that we support an<br>
empty list of indices with SpvOpAccessChain too.</blockquote><div><br></div><div>Yes, we do.  It's a bit of history at this point.  Originally, the pointer generated by OpVariable was a vtn_access_chain with a length of 0.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The specs do not state<br>
clearly whether this is valid or not though. Is it correct?<br><div class="HOEnZb"><div class="h5"></div></div></blockquote><div><br></div><div>Yeah, I'm not really sure what the rules are for OpAccessChain.  The SPIR-V spec is pretty bad about actually specifying what is and isn't valid SPIR-V.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> +<br>
> +      nir_ssa_def *elem_offset =<br>
> +         vtn_access_link_as_<wbr>ssa(b, deref_chain->link[idx],<br>
> +                             <wbr>   base->ptr_type->stride);<br>
> +      offset = nir_iadd(&b->nb, offset, elem_offset);<br>
> +      idx++;<br>
> +   }<br>
> +<br>
>     if (!block_index) {<br>
>        assert(base->var);<br>
>        if (glsl_type_is_array(type-><wbr>type)) {<br>
> @@ -1699,8 +1720,10 @@ vtn_handle_variables(struct vtn_builder *b,<br>
> SpvOp opcode,<br>
>     }<br>
>  <br>
>     case SpvOpAccessChain:<br>
> +   case SpvOpPtrAccessChain:<br>
>     case SpvOpInBoundsAccessChain: {<br>
>        struct vtn_access_chain *chain = vtn_access_chain_create(b,<br>
> count - 4);<br>
> +      chain->ptr_as_array = (opcode == SpvOpPtrAccessChain);<br>
>  <br>
>        unsigned idx = 0;<br>
>        for (int i = 4; i < count; i++) {<br>
</div></div></blockquote></div><br></div></div>