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

[Ian Romanick]

On 12/07/2017 02:26 PM, Jason Ekstrand wrote:
> On Thu, Dec 7, 2017 at 11:54 AM, Michael Schellenberger Costa
> <mschellenbergercosta at googlemail.com
> <mailto: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.

You could make a vtn_fail_opcode_if that takes the opcode as a
parameter.  I'm not sure if that would strictly be an improvement. *shrug*

>  
> 
>     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 "
> 
> 
> 
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>