<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 09/07/2017 07:15 PM, Jason Ekstrand wrote: </div> <blockquote type="cite" cite="mid:CAOFGe96Pz594NK-S2uCVRFcgiAhbOEBroX5c+XLbXy6c-xV7Og@mail.gmail.com"> <div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Aug 29, 2017 at 3:04 AM, Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com" target="_blank" moz-do-not-send="true">siglesias@igalia.com</a>> wrote: <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 offsets fields are null because there is not ssa to take the data from. However, we can dereference that pointer when processing an SpvOp*AccessChain opcodes through vtn_ssa_offset_pointer_dereference() when the OpVariable when the StorageClass is Uniform or StorageBuffer. Inside vtn_ssa_offset_pointer_dereference() we have the code to initialize block_index and offset if they are null, but it is called after checking if the pointer has then non-null. </blockquote> <div> </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. </div> <div> </div> </div> </div> </div> </blockquote> For example, one of the tests failing is: dEQP-VK.spirv_assembly.instruction.compute.indexing.opptraccesschain_u32 A snippet of its SPIR-V is: [...] OpDecorate %_ptr_buffer_Input ArrayStride 65536 [...] <div align="left"> %Input = OpTypeStruct %_arr__arr_mat4v4float_uint_32_uint_32 %_ptr_buffer_Input = OpTypePointer StorageBuffer %Input %dataInput = OpVariable %_ptr_buffer_Input StorageBuffer [...] %54 = OpPtrAccessChain %_ptr_buffer_float %dataInput %idx_1 %idx_0 %i0 %i1 %i2 %i3 </div> %dataInput points to a struct of an AoA of matrices. However, according to SPV_KHR_variable_pointers [0]: <ul> <li> Each OpPtrAccessChain must have a Base whose type is decorated with ArrayStride. </li> </ul> So I think this is fine, or am I understanding it wrongly? Sam [0] <a class="moz-txt-link-freetext" href="https://www.khronos.org/registry/spir-v/specs/1.2/SPIRV.html#OpPtrAccessChain">https://www.khronos.org/registry/spir-v/extensions/KHR/SPV_KHR_variable_pointers.html</a> <blockquote type="cite" cite="mid:CAOFGe96Pz594NK-S2uCVRFcgiAhbOEBroX5c+XLbXy6c-xV7Og@mail.gmail.com"> <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: dEQP-VK.spirv_assembly.instruction.*.indexing.* Signed-off-by: Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com" moz-do-not-send="true">siglesias@igalia.com</a>> --- src/compiler/spirv/vtn_variables.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 4f6acd2e07..baf1edde4c 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -146,20 +146,6 @@ vtn_ssa_offset_pointer_dereference(struct vtn_builder *b, struct vtn_type *type = base->type; unsigned idx = 0; - if (deref_chain->ptr_as_array) { - /* We need ptr_type for the stride */ - assert(base->ptr_type); - /* This must be a pointer to an actual element somewhere */ - assert(block_index && offset); - /* We need at least one element in the chain */ - assert(deref_chain->length >= 1); - - nir_ssa_def *elem_offset = - vtn_access_link_as_ssa(b, deref_chain->link[idx], - base->ptr_type->stride); - offset = nir_iadd(&b->nb, offset, elem_offset); - idx++; - } if (!block_index) { assert(base->var); @@ -182,6 +168,21 @@ vtn_ssa_offset_pointer_dereference(struct vtn_builder *b, } assert(offset); + if (deref_chain->ptr_as_array) { + /* We need ptr_type for the stride */ + assert(base->ptr_type); + /* This must be a pointer to an actual element somewhere */ + assert(block_index && offset); + /* We need at least one element in the chain */ + assert(deref_chain->length >= 1); + + nir_ssa_def *elem_offset = + vtn_access_link_as_ssa(b, deref_chain->link[idx], + base->ptr_type->stride); + offset = nir_iadd(&b->nb, offset, elem_offset); + idx++; + } + for (; idx < deref_chain->length; idx++) { switch (glsl_get_base_type(type->type)) { case GLSL_TYPE_UINT: -- 2.14.1 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" moz-do-not-send="true">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank" moz-do-not-send="true">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote> </div> </div> </div> </blockquote> </body> </html>