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

[Lionel Landwerlin]

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?
> +
> +   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);