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

[Bas Nieuwenhuizen]

Reviewed-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>

On Tue, Apr 24, 2018 at 7:55 AM, Neil Roberts <nroberts at igalia.com> 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,
> --
> 2.14.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev