On 01/02/2018 04:05 PM, Jason Ekstrand wrote:
> On January 2, 2018 15:22:51 Ian Romanick <i...@freedesktop.org> 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.c
>>> index 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.
"Same ID" is what I would have expected because it means the loader
doesn't have to do these deep checks. The rest of that background
information also matches my expectations. :)
> 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.
Yeah, that makes sense. I'm just thinking about how someone would
respond to seeing that warning come out of the driver. My initial
reaction would probably be, "Huh? They're both scalar, signed, 32-bit
integer. What gives?" Maybe changing the message to:
vtn_warn("Source and destination types of %s do not have the same "
"ID (but are compatible): %d vs %d",
spirv_op_to_string(opcode),
/* are the actual IDs even available here? */,
...);
> 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.
Ah... I overlooked that.
>>> + } 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
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
