Re: [Mesa-dev] [PATCH 3/3] spirv: Loosten the validation for load/store type matching

[Jason Ekstrand]

On January 2, 2018 11:59:15 Alejandro Piñeiro <apinhe...@igalia.com> wrote:
nitpick, from the commit message: is "loosten" an English word? Perhaps
do you mean "loosen"? (said the non-native English speaker).

Yes, Matt pointed that out as well.  I may be a native but my spelling 
ability is a bit sub-par some days...

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.

It's possible.  However, I think this approach is ok given that the rules 
we are actually enforcing (with the vtn_fail) is that the two types must be 
identical in their construction (minus decorations).  The primary reason we 
need this check at all is to ensure that loading and storing composite 
types works out ok.  Unless we get our source and destination tires mixed 
up somewhere, the decorations don't matter when in comes to compatibility 
of the resulting SSA value.

Could this change in the future?  Quite possibly.  If it does, then we will 
have to evaluate what checks we want to make.  There is a possibility that 
we may choose to stricter.  However, as it stands today, the apps are 
breaking the rules so anyone who sees that warning should go fix their app.

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