nitpick, from the commit message: is "loosten" an English word? Perhaps do you mean "loosen"? (said the non-native English speaker).
The patch looks good. My only concern is that as far as I understand, we are accepting non-strict SPIR-V shaders (quoting from previous patch: "Technically, the SPIR-V rules require the exact same type ID but this lets us internally be a bit looser."). I'm wondering if that could cause problems in the future if we are loosening the check for any shader, and this loosening should be conditional. Although perhaps that would be too much hassle right now. In any case, nitpicks and concerns apart: Reviewed-by: Alejandro Piñeiro <apinhe...@igalia.com> On 02/01/18 17:30, Jason Ekstrand wrote: > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104424 > --- > src/compiler/spirv/vtn_variables.c | 32 ++++++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/src/compiler/spirv/vtn_variables.c > b/src/compiler/spirv/vtn_variables.c > index d69b056..a74d0ce 100644 > --- a/src/compiler/spirv/vtn_variables.c > +++ b/src/compiler/spirv/vtn_variables.c > @@ -1899,6 +1899,29 @@ 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 (vtn_types_compatible(b, dst_type, src_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)); > + } 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 +1998,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 +2010,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 +2027,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 mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
