On January 2, 2018 15:22:51 Ian Romanick <[email protected]> wrote:
On 01/01/2018 08:09 PM, Jason Ekstrand wrote:Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104424 --- src/compiler/spirv/vtn_variables.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.cindex d69b056..48797f6 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c@@ -1899,6 +1899,28 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val,} } +static void +vtn_assert_types_equal(struct vtn_builder *b, SpvOp opcode, + struct vtn_type *dst_type, struct vtn_type *src_type) +{ + if (dst_type->val == src_type->val) + return; + + if (dst_type->type == src_type->type) { + /* Early versions of GLSLang would re-emit types unnecessarily and you + * would end up with OpLoad, OpStore, or OpCopyMemory opcodes which have + * mismatched source and destination types. + */ + vtn_warn("Source and destination types of %s do not match", + spirv_op_to_string(opcode));This is deep compare vs. shallow compare, right? Looking at the SPIR-V spec, it's not clear to me which kind of "equality" is necessary. What does the validator do? I'm just wondering of we should even bother emitting a warning since this may just be a sub-optimal SPIR-V binary. Emitting this particular warning here is a bit misleading. The real problem is that there are multiple identical types with different names, right?
That's an interesting question. The SPIR-V spec is definitely unclear on point and, if you wind back the clock a bit, I think you'll find that the working group didn't really have consensus for quite some time on what "the same type" really means. At this point in time, I believe the consensus is that it means the same SPIR-V id (in this patch we compare value pointers but that's equivalent). However, this consensus was reached long after the initial SPIR-V spec was released.
We certainly could ditch the warning and keep the vtn_fail_if only using a looser condition. That said, if the consensus is going to be that they must have the same id then I'm a fan of enforcing the rules even if it's just a warning.
Also, fyi, I sent a new version of this patch today which uses a new vtn_types_compatible helper which is a bit more obvious in it's behavior than comparing ->type.
+ } else { + vtn_fail("Source and destination types of %s do not match: %s vs. %s", + spirv_op_to_string(opcode), + glsl_get_type_name(dst_type->type), + glsl_get_type_name(src_type->type)); + } +} + void vtn_handle_variables(struct vtn_builder *b, SpvOp opcode, const uint32_t *w, unsigned count) @@ -1975,8 +1997,7 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp opcode, struct vtn_value *dest = vtn_value(b, w[1], vtn_value_type_pointer); struct vtn_value *src = vtn_value(b, w[2], vtn_value_type_pointer); - vtn_fail_if(dest->type->deref != src->type->deref, - "Dereferenced pointer types to OpCopyMemory do not match"); + vtn_assert_types_equal(b, opcode, dest->type->deref, src->type->deref); vtn_variable_copy(b, dest->pointer, src->pointer); break; @@ -1988,8 +2009,7 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp opcode, struct vtn_value *src_val = vtn_value(b, w[3], vtn_value_type_pointer); struct vtn_pointer *src = src_val->pointer; - vtn_fail_if(res_type != src_val->type->deref, - "Result and pointer types of OpLoad do not match"); + vtn_assert_types_equal(b, opcode, res_type, src_val->type->deref); if (src->mode == vtn_variable_mode_image || src->mode == vtn_variable_mode_sampler) { @@ -2006,8 +2026,7 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp opcode, struct vtn_pointer *dest = dest_val->pointer; struct vtn_value *src_val = vtn_untyped_value(b, w[2]); - vtn_fail_if(dest_val->type->deref != src_val->type, - "Value and pointer types of OpStore do not match"); + vtn_assert_types_equal(b, opcode, dest_val->type->deref, src_val->type); if (glsl_type_is_sampler(dest->type->type)) { vtn_warn("OpStore of a sampler detected. Doing on-the-fly copy "
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
