[Mesa-dev] [PATCH 3/8] spirv: Add basic type validation for OpLoad, OpStore, and OpCopyMemory

[Jason Ekstrand]

On Thu, Dec 7, 2017 at 11:54 AM, Michael Schellenberger Costa <
mschellenbergercosta at googlemail.com> wrote:

> Hi Jason,
>
>
> Am 07.12.2017 um 17:12 schrieb Jason Ekstrand:
>
>> ---
>>   src/compiler/spirv/vtn_variables.c | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/compiler/spirv/vtn_variables.c
>> b/src/compiler/spirv/vtn_variables.c
>> index cf44ed3..8ce19ff 100644
>> --- a/src/compiler/spirv/vtn_variables.c
>> +++ b/src/compiler/spirv/vtn_variables.c
>> @@ -1969,6 +1969,9 @@ 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,
>> +                  "Result and pointer types of OpLoad do not match");
>>
> This should be OpCopyMemory?


Oops.  Fixed locally.


> On a more general side: As you want to cover every OpCode, why not
> overload vtn_fail_if() so that it takes the OpCode and then prepends it to
> the error message, e.g.:
>
> vtn_fail_if(dest->type->deref != src->type->deref, opcode,"Result and
> pointer types of do not match");
>
> Would extend to
>
> "OpCodeMemory: Result and pointer types of do not match"
>
> That way there is no chance to really mess op the opcodes.
>

I'm not sure what I think about that.  There may be cases where we want to
use vtn_fail_if where we don't have ready access to the opcode.  One option
would be to use spirv_opcode_to_string instead of putting it in the string
but it may be hard to make that happen in such a way that we only call that
function if the vtn_fail condition is true.


> All the best
> Michael
>
>
> +
>>         vtn_variable_copy(b, dest->pointer, src->pointer);
>>         break;
>>      }
>> @@ -1976,8 +1979,11 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp
>> opcode,
>>      case SpvOpLoad: {
>>         struct vtn_type *res_type =
>>            vtn_value(b, w[1], vtn_value_type_type)->type;
>> -      struct vtn_pointer *src =
>> -         vtn_value(b, w[3], vtn_value_type_pointer)->pointer;
>> +      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");
>>           if (src->mode == vtn_variable_mode_image ||
>>             src->mode == vtn_variable_mode_sampler) {
>> @@ -1990,8 +1996,12 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp
>> opcode,
>>      }
>>        case SpvOpStore: {
>> -      struct vtn_pointer *dest =
>> -         vtn_value(b, w[1], vtn_value_type_pointer)->pointer;
>> +      struct vtn_value *dest_val = vtn_value(b, w[1],
>> vtn_value_type_pointer);
>> +      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");
>>           if (glsl_type_is_sampler(dest->type->type)) {
>>            vtn_warn("OpStore of a sampler detected.  Doing on-the-fly
>> copy "
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171207/8ae4b817/attachment.html>