Thanks Neil! Reviewed-by: Iago Toral Quiroga <ito...@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, _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
