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

[Samuel Iglesias Gonsálvez]

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?

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

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;
> > 
> 
>