[Mesa-dev] [PATCH 2/2] spirv: handle undefined components for OpVectorShuffle

[Lionel Landwerlin]

On 27/01/17 08:44, Juan A. Suarez Romero wrote:
> On Thu, 2017-01-26 at 17:08 +0000, Lionel Landwerlin wrote:
>> Fixes:
>>     dEQP-VK.spirv_assembly.instruction.compute.opspecconstantop.vector_related
>>     dEQP-VK.spirv_assembly.instruction.graphics.opspecconstantop.vector_related*
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> ---
>>   src/compiler/spirv/spirv_to_nir.c | 52 ++++++++++++++++++++++++++++-----------
>>   1 file changed, 38 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c
>> index 2d773b4373..8f39670f47 100644
>> --- a/src/compiler/spirv/spirv_to_nir.c
>> +++ b/src/compiler/spirv/spirv_to_nir.c
>> @@ -1113,23 +1113,43 @@ vtn_handle_constant(struct vtn_builder *b, SpvOp opcode,
>>         SpvOp opcode = get_specialization(b, val, w[3]);
>>         switch (opcode) {
>>         case SpvOpVectorShuffle: {
>> -         struct vtn_value *v0 = vtn_value(b, w[4], vtn_value_type_constant);
>> -         struct vtn_value *v1 = vtn_value(b, w[5], vtn_value_type_constant);
>> -         unsigned len0 = glsl_get_vector_elements(v0->const_type);
>> -         unsigned len1 = glsl_get_vector_elements(v1->const_type);
>> +         struct vtn_value *v0 = &b->values[w[4]];
>> +         struct vtn_value *v1 = &b->values[w[5]];
>> +
>> +         assert(v0->value_type == vtn_value_type_constant ||
>> +                v0->value_type == vtn_value_type_undef);
>> +         assert(v1->value_type == vtn_value_type_constant ||
>> +                v1->value_type == vtn_value_type_undef);
>> +
>> +         unsigned len0 = v0->value_type == vtn_value_type_constant ?
>> +                         glsl_get_vector_elements(v0->const_type) :
>> +                         glsl_get_vector_elements(v0->type->type);
>> +         unsigned len1 = v1->value_type == vtn_value_type_constant ?
>> +                         glsl_get_vector_elements(v1->const_type) :
>> +                         glsl_get_vector_elements(v1->type->type);
>>   
>
> Not sure if this is correct. Rather, I think the test is wrong.
>
> According to OpVectorShuffle spec[1], it is true that  it admits as
> operands either a constant or an OpUndef.
>
>
> But what the test does is calling OpSpecConstantOp[2], which is the
> operation we are patching here.
>
> And according to the spec, "all Operands must be the <id>s of other
> constant instructions", being constant instructions those starting with
> OpConstant or OpSpec. In this regard, OpUndef is not a constant.

I noticed this indeed. Given that test were specifically written to test 
this, I thought OpVectorShuffle had priority on this rule.

I just filed a bug against the spec to get clarification on this.

Thanks

>
>   
>>            assert(len0 + len1 < 16);
>>   
>>            unsigned bit_size = glsl_get_bit_size(val->const_type);
>> -         assert(bit_size == glsl_get_bit_size(v0->const_type) &&
>> -                bit_size == glsl_get_bit_size(v1->const_type));
>> +         unsigned bit_size0 = v0->value_type == vtn_value_type_constant ?
>> +                              glsl_get_bit_size(v0->const_type) :
>> +                              glsl_get_bit_size(v0->type->type);
>> +         unsigned bit_size1 = v1->value_type == vtn_value_type_constant ?
>> +                              glsl_get_bit_size(v1->const_type) :
>> +                              glsl_get_bit_size(v1->type->type);
>> +
>> +         assert(bit_size == bit_size0 && bit_size == bit_size1);
>>   
>>            if (bit_size == 64) {
>>               uint64_t u64[8];
>> -            for (unsigned i = 0; i < len0; i++)
>> -               u64[i] = v0->constant->values[0].u64[i];
>> -            for (unsigned i = 0; i < len1; i++)
>> -               u64[len0 + i] = v1->constant->values[0].u64[i];
>> +            for (unsigned i = 0; i < len0; i++) {
>> +               if (v0->value_type == vtn_value_type_constant)
>> +                  u64[i] = v0->constant->values[0].u64[i];
>> +            }
>> +            for (unsigned i = 0; i < len1; i++) {
>> +               if (v1->value_type == vtn_value_type_constant)
>> +                  u64[len0 + i] = v1->constant->values[0].u64[i];
>> +            }
>>   
>>               for (unsigned i = 0, j = 0; i < count - 6; i++, j++) {
>>                  uint32_t comp = w[i + 6];
>> @@ -1143,11 +1163,15 @@ vtn_handle_constant(struct vtn_builder *b, SpvOp opcode,
>>               }
>>            } else {
>>               uint32_t u32[8];
>> -            for (unsigned i = 0; i < len0; i++)
>> -               u32[i] = v0->constant->values[0].u32[i];
>> +            for (unsigned i = 0; i < len0; i++) {
>> +               if (v0->value_type == vtn_value_type_constant)
>> +                  u32[i] = v0->constant->values[0].u32[i];
>> +            }
>>   
>> -            for (unsigned i = 0; i < len1; i++)
>> -               u32[len0 + i] = v1->constant->values[0].u32[i];
>> +            for (unsigned i = 0; i < len1; i++) {
>> +               if (v1->value_type == vtn_value_type_constant)
>> +                  u32[len0 + i] = v1->constant->values[0].u32[i];
>> +            }
>>   
>>               for (unsigned i = 0, j = 0; i < count - 6; i++, j++) {
>>                  uint32_t comp = w[i + 6];
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev