[Mesa-dev] [PATCH 2/2] spirv: fix bug when OpSpecConstantOp calls a conversion

[Ian Romanick]

On 11/29/2017 02:17 AM, Samuel Iglesias Gonsálvez wrote:
> On Tue, 2017-11-28 at 13:13 -0800, Ian Romanick wrote:
>> On 11/20/2017 10:25 PM, Samuel Iglesias Gonsálvez wrote:
>>> In that case, nir_eval_const_opcode() will evaluate the conversion
>>> but as it was using destination's bit_size, the resulting
>>> value was just a cast of the source constant value. By passing the
>>> source's bit size, it does the conversion properly.
>>>
>>> Fixes:
>>>
>>> dEQP-VK.spirv_assembly.instruction.*.opspecconstantop.*convert*
>>>
>>> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>>> ---
>>>  src/compiler/spirv/spirv_to_nir.c | 31 ++++++++++++++++++++++++++-
>>> ----
>>>  1 file changed, 26 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/compiler/spirv/spirv_to_nir.c
>>> b/src/compiler/spirv/spirv_to_nir.c
>>> index 2cc3c275ea9..ffbda4f1946 100644
>>> --- a/src/compiler/spirv/spirv_to_nir.c
>>> +++ b/src/compiler/spirv/spirv_to_nir.c
>>> @@ -1348,14 +1348,35 @@ vtn_handle_constant(struct vtn_builder *b,
>>> SpvOp opcode,
>>>           bool swap;
>>>           nir_alu_type dst_alu_type =
>>> nir_get_nir_type_for_glsl_type(val->const_type);
>>>           nir_alu_type src_alu_type = dst_alu_type;
>>> -         nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode,
>>> &swap, src_alu_type, dst_alu_type);
>>> -
>>>           unsigned num_components = glsl_get_vector_elements(val-
>>>> const_type);
>>> -         unsigned bit_size =
>>> -            glsl_get_bit_size(val->const_type);
>>> +         unsigned bit_size;
>>>  
>>> -         nir_const_value src[4];
>>>           assert(count <= 7);
>>> +
>>> +         switch (opcode) {
>>> +         case SpvOpUConvert:
>>> +         case SpvOpConvertFToU:
>>> +         case SpvOpConvertFToS:
>>> +         case SpvOpConvertSToF:
>>> +         case SpvOpConvertUToF:
>>> +         case SpvOpSConvert:
>>> +         case SpvOpFConvert:
>>
>> I'm not sure what we should do here.  Most of these opcodes are not
>> valid in SpvOpSpecConstantOp in a Shader.  Only SpvOpSConvert and
>> SpvOpFConvert are allowed.  I guess it's trivial enough to support
>> the
>> others anyway...
> 
> Right.
> 
>>  do those dEQP-VK.spirv_assembly.instruction.* tests
>> exercise the other opcodes?
>>
> 
> No, they only exercise SpvOpFConvert and SpvOpSConvert. Do you prefer
> to just support these two opcodes for the moment?

Okay... good.  I wanted to make sure they weren't expecting out-of-spec
behavior. :)

> With or without that change, does it get your R-b?

Yes.

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> 
> Sam
> 
>>> +            /* We have a source in a conversion */
>>> +            src_alu_type =
>>> +               nir_get_nir_type_for_glsl_type(
>>> +                  vtn_value(b, w[4], vtn_value_type_constant)-
>>>> const_type);
>>> +            /* We use the bitsize of the conversion source to
>>> evaluate the opcode later */
>>> +            bit_size = glsl_get_bit_size(
>>> +               vtn_value(b, w[4], vtn_value_type_constant)-
>>>> const_type);
>>> +            break;
>>> +         default:
>>> +            bit_size = glsl_get_bit_size(val->const_type);
>>> +         };
>>> +
>>> +
>>> +         nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode,
>>> &swap, src_alu_type, dst_alu_type);
>>> +         nir_const_value src[4];
>>> +
>>>           for (unsigned i = 0; i < count - 4; i++) {
>>>              nir_constant *c =
>>>                 vtn_value(b, w[4 + i], vtn_value_type_constant)-
>>>> constant;
>>>