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

[Iago Toral]

On Tue, 2016-11-22 at 12:15 +, Lionel Landwerlin wrote:
> > 
> Sounds good to me, thanks!
> 
> Reviewed-by: Lionel Landwerlin 
> 
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(>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, );
> > +
> > +  if (swap) {
> > + nir_ssa_def *tmp = src[0];
> > + src[0] = src[1];
> > + src[1] = tmp;
> > +  }
> > +
> > +  val->ssa->def =
> > + nir_ior(>nb,
> > + nir_build_alu(>nb, op, src[0], src[1], NULL,
> > NULL),
> > + nir_ior(>nb,
> > + nir_fne(>nb, src[0], src[0]),
> > + nir_fne(>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, );
> > +
> > +  if (swap) {
> > + nir_ssa_def *tmp = src[0];
> > + src[0] = src[1];
> > + src[1] = tmp;
> > +  }
> > +
> > +  val->ssa->def =
> > + nir_iand(>nb,
> > +  nir_build_alu(>nb, op, src[0], src[1], NULL,
> > NULL),
> > +  nir_iand(>nb,
> > +  nir_feq(>nb, src[0], src[0]),
> > +  nir_feq(>nb, src[1], src[1])));
> > +  break;
> > +   }
> > +
> >  default: {
> > bool swap;
> > nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode, );
> 
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Lionel Landwerlin]

Sounds good to me, thanks!

Reviewed-by: Lionel Landwerlin 

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(>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, );
+
+  if (swap) {
+ nir_ssa_def *tmp = src[0];
+ src[0] = src[1];
+ src[1] = tmp;
+  }
+
+  val->ssa->def =
+ nir_ior(>nb,
+ nir_build_alu(>nb, op, src[0], src[1], NULL, NULL),
+ nir_ior(>nb,
+ nir_fne(>nb, src[0], src[0]),
+ nir_fne(>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, );
+
+  if (swap) {
+ nir_ssa_def *tmp = src[0];
+ src[0] = src[1];
+ src[1] = tmp;
+  }
+
+  val->ssa->def =
+ nir_iand(>nb,
+  nir_build_alu(>nb, op, src[0], src[1], NULL, NULL),
+  nir_iand(>nb,
+  nir_feq(>nb, src[0], src[0]),
+  nir_feq(>nb, src[1], src[1])));
+  break;
+   }
+
 default: {
bool swap;
nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode, );



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev