[Mesa-dev] [PATCH] spirv: Don’t check for NaN for most OpFOrd* comparisons
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] spirv: Don’t check for NaN for most OpFOrd* comparisons
Reviewed-by: Bas Nieuwenhuizen
On Tue, Apr 24, 2018 at 7:55 AM, 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,
> --
> 2.14.3
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] spirv: Don’t check for NaN for most OpFOrd* comparisons
Thanks Neil!
Reviewed-by: Iago Toral Quiroga
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,
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
