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

Fri Dec 7 00:16:11 UTC 2018

On 12/05/2018 10:12 AM, Connor Abbott 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

These two optimizations do not play well together.  Each by itself is
valid, but the combination is not.  As I mentioned in the previous
thread, I believe the first change should mark the resulting (op, a, b)
as precise.  That would prevent the second optimization from triggering.
 I don't think nir_opt_algebraic has way to do this, but it doesn't seem
like it would be that hard to add.  Regardless of what happens with the
rest, breaking the cumulative effect of these optimizations is necessary.

> 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.

Even with the work that I've been doing this week, removing these
optimizations still hurts (quite dramatically in a few cases) over 1500
shaders in shader-db.  By and large, applications go to great lengths to
avoid things that could generate NaN.  If a calculation generates NaN in
a graphics shader, you've already lost.  As a result, these hurt shaders
work as-is.  Adding an extra 8% instructions to a working shader doesn't
help anyone.

Based on the commit message in d55835b8bdf0, my hypothesis is that
disabling the combination of the two sets of optimizations won't
negatively affect any shaders.

> - (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
> 