[Mesa-dev] [PATCH v2] nir/spirv: implement ordered / unordered floating point comparisons properly

[Iago Toral]

On Tue, 2016-11-22 at 12:15 +0000, Lionel Landwerlin wrote:
> > 
> Sounds good to me, thanks!
> 
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> 
On 17/11/16 08:36, Iago Toral Quiroga wrote:
> > 
> > Besides the logical operation involved, these also require that we
> > test if the
> > operands are ordered / unordered.
> > 
> > For ordered operations, both operands must be ordered (and they
> > must pass the
> > conditional test) while for unordered operations it is sufficient
> > if only one
> > of the operands is unordered (or they pass the logical test).
> > 
> > Fixes the following Vulkan CTS tests:
> > 
> > dEQP-VK.spirv_assembly.instruction.compute.opfunord.equal
> > dEQP-VK.spirv_assembly.instruction.compute.opfunord.greater
> > dEQP-VK.spirv_assembly.instruction.compute.opfunord.greaterequal
> > dEQP-VK.spirv_assembly.instruction.compute.opfunord.less
> > dEQP-VK.spirv_assembly.instruction.compute.opfunord.lessequal
> > 
> > v2: Fixed typo: s/nir_eq/nir_feq
> > ---
> >   src/compiler/spirv/vtn_alu.c | 54
> > +++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 53 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/compiler/spirv/vtn_alu.c
> > b/src/compiler/spirv/vtn_alu.c
> > index 6d98a62..6d97f6c 100644
> > --- a/src/compiler/spirv/vtn_alu.c
> > +++ b/src/compiler/spirv/vtn_alu.c
> > @@ -257,7 +257,10 @@ vtn_nir_alu_op_for_spirv_opcode(SpvOp opcode,
> > bool *swap)
> >      case SpvOpBitReverse:            return
> > nir_op_bitfield_reverse;
> >      case SpvOpBitCount:              return nir_op_bit_count;
> >   
> > -   /* Comparisons: (TODO: How do we want to handled
> > ordered/unordered?) */
> > +   /* The ordered / unordered operators need special
> > implementation besides
> > +    * the logical operator to use since they also need to check if
> > operands are
> > +    * ordered.
> > +    */
> >      case SpvOpFOrdEqual:                            return
> > nir_op_feq;
> >      case SpvOpFUnordEqual:                          return
> > nir_op_feq;
> >      case SpvOpINotEqual:                            return
> > nir_op_ine;
> > @@ -447,6 +450,55 @@ vtn_handle_alu(struct vtn_builder *b, SpvOp
> > opcode,
> >                                         nir_imm_float(&b->nb,
> > INFINITY));
> >         break;
> >   
> Maybe drop the additional empty line?

Yeah, that should not be there, I'll drop it before pushing. Thanks!

Iago

> > +
> > +   case SpvOpFUnordEqual:
> > +   case SpvOpFUnordNotEqual:
> > +   case SpvOpFUnordLessThan:
> > +   case SpvOpFUnordGreaterThan:
> > +   case SpvOpFUnordLessThanEqual:
> > +   case SpvOpFUnordGreaterThanEqual: {
> > +      bool swap;
> > +      nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode, &swap);
> > +
> > +      if (swap) {
> > +         nir_ssa_def *tmp = src[0];
> > +         src[0] = src[1];
> > +         src[1] = tmp;
> > +      }
> > +
> > +      val->ssa->def =
> > +         nir_ior(&b->nb,
> > +                 nir_build_alu(&b->nb, op, src[0], src[1], NULL,
> > NULL),
> > +                 nir_ior(&b->nb,
> > +                         nir_fne(&b->nb, src[0], src[0]),
> > +                         nir_fne(&b->nb, src[1], src[1])));
> > +      break;
> > +   }
> > +
> > +   case SpvOpFOrdEqual:
> > +   case SpvOpFOrdNotEqual:
> > +   case SpvOpFOrdLessThan:
> > +   case SpvOpFOrdGreaterThan:
> > +   case SpvOpFOrdLessThanEqual:
> > +   case SpvOpFOrdGreaterThanEqual: {
> > +      bool swap;
> > +      nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode, &swap);
> > +
> > +      if (swap) {
> > +         nir_ssa_def *tmp = src[0];
> > +         src[0] = src[1];
> > +         src[1] = tmp;
> > +      }
> > +
> > +      val->ssa->def =
> > +         nir_iand(&b->nb,
> > +                  nir_build_alu(&b->nb, op, src[0], src[1], NULL,
> > NULL),
> > +                  nir_iand(&b->nb,
> > +                          nir_feq(&b->nb, src[0], src[0]),
> > +                          nir_feq(&b->nb, src[1], src[1])));
> > +      break;
> > +   }
> > +
> >      default: {
> >         bool swap;
> >         nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode, &swap);
> 
>