[Mesa-dev] [PATCH] spirv: Don’t check for NaN for most OpFOrd* comparisons

[Iago Toral]

Thanks Neil!

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

Maybe we need other drivers (radv?) to double-check that this doesn't
break stuff for them either?

Iago

On Tue, 2018-04-24 at 16:55 +0200, Neil Roberts wrote:
> For all of the OpFOrd* comparisons except OpFOrdNotEqual the hardware
> should probably already return false if one of the operands is NaN so
> we don’t need to have an explicit check for it. This seems to at
> least
> work on Intel hardware. This should reduce the number of instructions
> generated for the most common comparisons.
> 
> For what it’s worth, the original code to handle this was added in
> e062eb6415de3a. The commit message for that says that it was to fix
> some CTS tests for OpFUnord* opcodes. Even if the hardware doesn’t
> handle NaNs this patch shouldn’t affect those tests. At any rate they
> have since been moved out of the mustpass list. Incidentally those
> tests fail on the nvidia proprietary driver so it doesn’t seem like
> handling NaNs correctly is a priority.
> ---
> 
> I made a VkRunner test case for all of the OpFOrd* and OpFUnord*
> opcodes with and without NaNs on the test branch. It can be run like
> this:
> 
> git clone -b tests https://github.com/Igalia/vkrunner.git
> cd vkrunner
> ./autogen.sh && make -j8
> ./src/vkrunner examples/unordered-comparison.shader_test
> 
>  src/compiler/spirv/vtn_alu.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/src/compiler/spirv/vtn_alu.c
> b/src/compiler/spirv/vtn_alu.c
> index 71e743cdd1e..3134849ba90 100644
> --- a/src/compiler/spirv/vtn_alu.c
> +++ b/src/compiler/spirv/vtn_alu.c
> @@ -597,23 +597,18 @@ vtn_handle_alu(struct vtn_builder *b, SpvOp
> opcode,
>        break;
>     }
>  
> -   case SpvOpFOrdEqual:
> -   case SpvOpFOrdNotEqual:
> -   case SpvOpFOrdLessThan:
> -   case SpvOpFOrdGreaterThan:
> -   case SpvOpFOrdLessThanEqual:
> -   case SpvOpFOrdGreaterThanEqual: {
> +   case SpvOpFOrdNotEqual: {
> +      /* For all the SpvOpFOrd* comparisons apart from NotEqual, the
> value
> +       * from the ALU will probably already be false if the operands
> are not
> +       * ordered so we don’t need to handle it specially.
> +       */
>        bool swap;
>        unsigned src_bit_size = glsl_get_bit_size(vtn_src[0]->type);
>        unsigned dst_bit_size = glsl_get_bit_size(type);
>        nir_op op = vtn_nir_alu_op_for_spirv_opcode(b, opcode, &swap,
>                                                    src_bit_size,
> dst_bit_size);
>  
> -      if (swap) {
> -         nir_ssa_def *tmp = src[0];
> -         src[0] = src[1];
> -         src[1] = tmp;
> -      }
> +      assert(!swap);
>  
>        val->ssa->def =
>           nir_iand(&b->nb,