<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Sep 11, 2017 at 1:42 AM, Samuel Iglesias Gonsálvez <span dir="ltr"><<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@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 text="#000000" bgcolor="#FFFFFF"><span class="">
<p><br>
</p>
<br>
<div class="m_-666671271335699196moz-cite-prefix">On 09/07/2017 07:15 PM, Jason Ekstrand
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">On Tue, Aug 29, 2017 at 3:04 AM,
Samuel Iglesias Gonsálvez <span dir="ltr"><<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">When
creating a vtn_pointer for an OpVariable, the block_index
and<br>
offsets fields are null because there is not ssa to take
the data<br>
from.<br>
<br>
However, we can dereference that pointer when processing
an<br>
SpvOp*AccessChain opcodes through vtn_ssa_offset_pointer_derefer<wbr>ence()<br>
when the OpVariable when the StorageClass is Uniform or
StorageBuffer.<br>
<br>
Inside vtn_ssa_offset_pointer_derefer<wbr>ence() we have
the code to<br>
initialize block_index and offset if they are null, but it
is called<br>
after checking if the pointer has then non-null.<br>
</blockquote>
<div><br>
</div>
<div>This seems fishy. The code you're moving only triggers
for OpPtrAccessChain. In order to run into an issue, they
would have to be doing an OpPtrAccessChain an an array of
resources. This shouldn't be allowed by the spec. I'll
have to look at the actual SPIR-V to be sure, but I think
this is probably a CTS bug.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br></span>
For example, one of the tests failing is:
dEQP-VK.spirv_assembly.<wbr>instruction.compute.indexing.<wbr>opptraccesschain_u32<br></div></blockquote><div><br></div><div>Did a little looking at this test in particular and it's definitely bogus.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
A snippet of its SPIR-V is:<br>
<br>
[...]<br>
OpDecorate %_ptr_buffer_Input ArrayStride 65536<br>
[...]<br>
<div align="left"> %Input = OpTypeStruct
%_arr__arr_mat4v4float_uint_<wbr>32_uint_32<br></div></div></blockquote><div><br></div><div>This is a struct containing an array of arrays. The struct is decorated with Block which is reasonable<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><div align="left">
%_ptr_buffer_Input = OpTypePointer StorageBuffer %Input<br></div></div></blockquote><div><br></div><div>This is a pointer to that struct. In the full SPIR-V, it's decorated with an array stride which makes no sense whatsoever because you can't just jump from one block to the next with a stride. It's possible that the test author intended to have the block contain an implicit array of structs but if they wanted that then they should have done a struct of array of struct of array of array of matrices.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><div align="left">
%dataInput = OpVariable %_ptr_buffer_Input StorageBuffer<br>
[...]<br>
%54 = OpPtrAccessChain %_ptr_buffer_float %dataInput %idx_1 %idx_0
%i0 %i1 %i2 %i3<br>
<br>
</div>
%dataInput points to a struct of an AoA of matrices. However,
according to SPV_KHR_variable_pointers [0]: <br>
<ul>
<li>
<p>
Each <b>OpPtrAccessChain</b> must have a <i>Base</i>
whose type is decorated with <b>ArrayStride</b>.</p></li></ul></div></blockquote><div> </div><div>That is the rule that *should* be catching this but they got around it by slapping a bogus ArrayStride decoration onto it. I'm not really sure what SPIR-V rule is supposed to catch this. One of the design flaws of SPIR-V is that we have valid usage rules based on decorations like you quoted above but we have no way of saying that %_arr__arr_mat4v4float_uint_32_uint_32 can't have an ArrayStride decoration. In fact, it can have such a decoration and might have to if it's also embedded in an array somewhere. I'll file SPIR-V and CTS bugs. I really wish we had caught this before the test got merged...</div><div><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><ul><li><p>
</p>
</li>
</ul>
So I think this is fine, or am I understanding it wrongly?<br>
<br>
Sam<br>
<br>
[0] <a class="m_-666671271335699196moz-txt-link-freetext" href="https://www.khronos.org/registry/spir-v/specs/1.2/SPIRV.html#OpPtrAccessChain" target="_blank">https://www.khronos.org/<wbr>registry/spir-v/extensions/<wbr>KHR/SPV_KHR_variable_pointers.<wbr>html</a><div><div class="h5"><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Reordering that code fixes crashes in:<br>
<br>
dEQP-VK.spirv_assembly.instru<wbr>ction.*.indexing.*<br>
<br>
Signed-off-by: Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>><br>
---<br>
src/compiler/spirv/vtn_variab<wbr>les.c | 29
+++++++++++++++--------------<br>
1 file changed, 15 insertions(+), 14 deletions(-)<br>
<br>
diff --git a/src/compiler/spirv/vtn_varia<wbr>bles.c
b/src/compiler/spirv/vtn_varia<wbr>bles.c<br>
index 4f6acd2e07..baf1edde4c 100644<br>
--- a/src/compiler/spirv/vtn_varia<wbr>bles.c<br>
+++ b/src/compiler/spirv/vtn_varia<wbr>bles.c<br>
@@ -146,20 +146,6 @@ vtn_ssa_offset_pointer_derefer<wbr>ence(struct
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->length >= 1);<br>
-<br>
- nir_ssa_def *elem_offset =<br>
- vtn_access_link_as_ssa(b,
deref_chain->link[idx],<br>
-
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>
@@ -182,6 +168,21 @@ vtn_ssa_offset_pointer_derefer<wbr>ence(struct
vtn_builder *b,<br>
}<br>
assert(offset);<br>
<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->length >= 1);<br>
+<br>
+ nir_ssa_def *elem_offset =<br>
+ vtn_access_link_as_ssa(b,
deref_chain->link[idx],<br>
+
base->ptr_type->stride);<br>
+ offset = nir_iadd(&b->nb, offset,
elem_offset);<br>
+ idx++;<br>
+ }<br>
+<br>
for (; idx < deref_chain->length; idx++) {<br>
switch (glsl_get_base_type(type->type<wbr>)) {<br>
case GLSL_TYPE_UINT:<br>
<span class="m_-666671271335699196HOEnZb"><font color="#888888">--<br>
2.14.1<br>
<br>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div></div>