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