[Mesa-dev] [PATCH 1/2] nir: add a compiler option for disabling float comparison simplifications

[Samuel Pitoiset]

On 12/1/18 3:36 PM, Connor Abbott wrote:
> On Sat, Dec 1, 2018 at 3:22 PM Samuel Pitoiset
> <samuel.pitoiset at gmail.com> wrote:
>>
>> I'm not saying this series is the right thing to do. It just fixes two
>> test failures in the vkd3d testsuite for RADV. I added a new compiler
>> option to not break anything and to only affects RADV. Anyways, it seems
>> unclear what the best option is. To sum up, looks like there is 3 ways:
>>
>> 1) set the exact bit for all SPIRV float comparisons ops
>> 2) draft a new extension, not sure if we really need to
>> 3) just remove these optimisations when targeting Vulkan
>>
>> Opinions are welcome, thanks!
> 
> Well, I was just pointing out that this has bit us in the past, and
> will probably bite us in the future if we don't fix it once and for
> all, so it's about more than the vkd3d testsuite. We're just getting
> lucky since right now the optimizations only trigger and mess things
> up with this testsuite. After thinking about it, I think it's best if
> we do another option 4, which is to remove the optimizations in
> question and always do the right thing even for GLSL.

I agree, it's not the first time we have problems with that and it would 
be very nice to fix it for real. vkd3d is just one example.

> 
>>
>> On 11/29/18 4:22 PM, Jason Ekstrand wrote:
>>> Can you provide some context for this?  Those rules are already flagged
>>> "inexact" (that's what the ~ means) so they won't apply to anything
>>> that's "precise" or "invariant".
>>>
>>> On Thu, Nov 29, 2018 at 9:18 AM Samuel Pitoiset
>>> <samuel.pitoiset at gmail.com <mailto:samuel.pitoiset at gmail.com>> wrote:
>>>
>>>      It's correct in GLSL because the behaviour is undefined in
>>>      presence of NaNs. But this seems incorrect in Vulkan.
>>>
>>>      Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com
>>>      <mailto:samuel.pitoiset at gmail.com>>
>>>      ---
>>>        src/compiler/nir/nir.h                | 6 ++++++
>>>        src/compiler/nir/nir_opt_algebraic.py | 8 ++++----
>>>        2 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>>      diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>>>      index db935c8496b..4107c293962 100644
>>>      --- a/src/compiler/nir/nir.h
>>>      +++ b/src/compiler/nir/nir.h
>>>      @@ -2188,6 +2188,12 @@ typedef struct nir_shader_compiler_options {
>>>           /* Set if nir_lower_wpos_ytransform() should also invert
>>>      gl_PointCoord. */
>>>           bool lower_wpos_pntc;
>>>
>>>      +       /* If false, lower ~inot(flt(a,b)) -> fge(a,b) and variants.
>>>      +        * In presence of NaNs, this is correct in GLSL because the
>>>      behaviour is
>>>      +        * undefined. In Vulkan, doing these transformations is
>>>      incorrect.
>>>      +        */
>>>      +       bool exact_float_comparisons;
>>>      +
>>>           /**
>>>            * Should nir_lower_io() create load_interpolated_input intrinsics?
>>>            *
>>>      diff --git a/src/compiler/nir/nir_opt_algebraic.py
>>>      b/src/compiler/nir/nir_opt_algebraic.py
>>>      index f2a7be0c403..3750874407b 100644
>>>      --- a/src/compiler/nir/nir_opt_algebraic.py
>>>      +++ b/src/compiler/nir/nir_opt_algebraic.py
>>>      @@ -154,10 +154,10 @@ optimizations = [
>>>           (('ishl', ('imul', a, '#b'), '#c'), ('imul', a, ('ishl', b, c))),
>>>
>>>           # 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)),
>>>      +   (('~inot', ('flt', a, b)), ('fge', a, b),
>>>      '!options->exact_float_comparisons'),
>>>      +   (('~inot', ('fge', a, b)), ('flt', a, b),
>>>      '!options->exact_float_comparisons'),
>>>      +   (('~inot', ('feq', a, b)), ('fne', a, b),
>>>      '!options->exact_float_comparisons'),
>>>      +   (('~inot', ('fne', a, b)), ('feq', a, b),
>>>      '!options->exact_float_comparisons'),
>>>
>>>
>>> The feq/fne one is actually completely safe.  fne is defined to be !feq
>>> even when NaN is considered.
>>>
>>> --Jasoan
>>>
>>>           (('inot', ('ilt', a, b)), ('ige', a, b)),
>>>           (('inot', ('ult', a, b)), ('uge', a, b)),
>>>           (('inot', ('ige', a, b)), ('ilt', a, b)),
>>>      --
>>>      2.19.2
>>>
>>>      _______________________________________________
>>>      mesa-dev mailing list
>>>      mesa-dev at lists.freedesktop.org <mailto: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