[Mesa-dev] [PATCH 05/28] Revert "spirv: Don’t check for NaN for most OpFOrd* comparisons"

[Jason Ekstrand]

I sent a series last week which does almost everything Connor mentioned...

On December 5, 2018 12:12:50 Connor Abbott <cwabbott0 at gmail.com> wrote:

> This won't work, since this optimization in nir_opt_algebraic will undo it:
>
> # For any float comparison operation, "cmp", if you have "a == a && a cmp b"
> # then the "a == a" is redundant because it's equivalent to "a is not NaN"
> # and, if a is a NaN then the second comparison will fail anyway.
> for op in ['flt', 'fge', 'feq']:
>   optimizations += [
>      (('iand', ('feq', a, a), (op, a, b)), (op, a, b)),
>      (('iand', ('feq', a, a), (op, b, a)), (op, b, a)),
>   ]
>
> and then this optimization might change the behavior for NaN's:
>
>   # Comparison simplifications
>   (('~inot', ('flt', a, b)), ('fge', a, b)),
>   (('~inot', ('fge', a, b)), ('flt', a, b)),
>   (('~inot', ('feq', a, b)), ('fne', a, b)),
>   (('~inot', ('fne', a, b)), ('feq', a, b)),
>
> The topic of NaN's and comparisons in NIR has been discussed several
> times before, most recently with this thread:
> https://lists.freedesktop.org/archives/mesa-dev/2018-December/210780.html
>
> Given that this language got added to the Vulkan spec: "By default,
> the implementation may perform optimizations on half, single, or
> double-precision floating-point instructions respectively that ignore
> sign of a zero, or assume that arguments and results are not Nans or
> ±∞" I think we should probably do the following:
>
> - Fix the CTS tests that prompted this whole block of code to only
> check the result of comparing NaN when this extension is available and
> NaN's are preserved.
> - nir_op_fne must be unordered already, regardless of what
> floating-point options the user asked for, since it's used to
> implement isNan() already. We should probably also define nir_op_feq
> to be ordered. So we don't have to do anything with them, and !(a ==
> b) == (a == b) is guaranteed.
> - Define fgt and fle to be ordered, as this is what every backend
> already does. No need to add unnecessary NaN checks.
> - Disable the comparison simplifications (except for the fne one,
> which shouldn't be marked imprecise as it is now) when preserve-nan is
> set. I think there are a few other imprecise transforms that also need
> to be disabled.
> - (optional) Add fgtu and fleu opcodes for unordered comparisons. This
> might help backends which can do these in only one instruction. Even
> if we don't do this, these can be implemented as not (fle a, b) and
> not (fgt a, b) respectively, which is fewer instructions than the
> current lowering.
> - (optional) Add fequ and fneo opcodes that do unordered equal and
> ordered not-equal, respectively. Otherwise they have to be implemented
> with explicit NaN checks like now.
>
> On Wed, Dec 5, 2018 at 4:56 PM Samuel Iglesias Gonsálvez
> <siglesias at igalia.com> wrote:
>>
>> This reverts commit c4ab1bdcc9710e3c7cc7115d3be9c69b7e7712ef. We need
>> to check the arguments looking for NaNs, because they can introduce
>> failures in tests for FOrd*, specially when running
>> VK_KHR_shader_float_control tests in CTS.
>>
>> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>> ---
>>  src/compiler/spirv/vtn_alu.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/compiler/spirv/vtn_alu.c b/src/compiler/spirv/vtn_alu.c
>> index dc6fedc9129..629b57560ca 100644
>> --- a/src/compiler/spirv/vtn_alu.c
>> +++ b/src/compiler/spirv/vtn_alu.c
>> @@ -535,18 +535,23 @@ vtn_handle_alu(struct vtn_builder *b, SpvOp opcode,
>>        break;
>>     }
>>
>> -   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.
>> -       */
>> +   case SpvOpFOrdEqual:
>> +   case SpvOpFOrdNotEqual:
>> +   case SpvOpFOrdLessThan:
>> +   case SpvOpFOrdGreaterThan:
>> +   case SpvOpFOrdLessThanEqual:
>> +   case SpvOpFOrdGreaterThanEqual: {
>>        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);
>>
>> -      assert(!swap);
>> +      if (swap) {
>> +         nir_ssa_def *tmp = src[0];
>> +         src[0] = src[1];
>> +         src[1] = tmp;
>> +      }
>>
>>        val->ssa->def =
>>           nir_iand(&b->nb,
>> --
>> 2.19.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev