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

Sat Dec 1 13:58:32 UTC 2018

On Fri, Nov 30, 2018 at 10:18 PM Ian Romanick <idr at freedesktop.org> wrote:
>
> On 11/29/2018 07:47 AM, Connor Abbott wrote:
> > On Thu, Nov 29, 2018 at 4:22 PM Jason Ekstrand <jason at jlekstrand.net> 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".
> >
> > I think the concern is that this isn't allowed in SPIR-V, even without
> > exact or invariant. We even go out of our way to do the correct thing
> > in the frontend by inserting an "&& a == a" or "|| a != a", but then
>
> If you're that paranoid about it, why not just mark the operations are
> precise?  That's literally why it exists.

Yes, that's right. And if we decide we need to get it correct for
SPIR-V, then what it's doing now is broken anyways...

>
> > opt_algebraic removes it with another rule and then this rule can flip
> > it from ordered to unordered. The spec says that operations don't have
> > to produce NaN, but it doesn't say anything on comparisons other than
> > the generic "everything must follow IEEE rules" and an entry in the
> > table that says "produces correct results." Then again, I can't find
> > anything in GLSL allowing these transforms either, so maybe we just
> > need to get rid of them.
>
> What I hear you saying is, "The behavior isn't defined."  Unless you can
> point to a CTS test or an application that has incorrect behavior, I'm
> going to oppose removing this pretty strongly.  *Every* GLSL compiler
> does this.

No, I don't think ARB_shader_precision definitely says that the
behavior is undefined. While it does say that you don't have to
produce NaN's, it also says that intBitsToFloat() must produce a NaN
given the right input, it otherwise just says that comparisons must
produce the "correct result," with no exception for NaN's. "correct
result" does not mean "the behavior is undefined." It never refers
back to the IEEE spec or says what "correct result" means, but one
could only assume it's referring to the required unsignaling
comparisons (Table 5.1 and 5.3 in IEEE 754-2008), which is also what C
defines them to be. Those rules haven't changed much since, and
they're basically the same for Vulkan.

As have others have said, there are currently Vulkan CTS tests that
actually checks comparisons with NaN, and we currently pass it
basically by dumb luck because of the brokenness I mentioned (see mesa
commit e062eb6415de3aa51b43f30d638ce8215efc0511 which introduced the
extra checks for NaN and cites the CTS tests). It would probably be an
uphill battle to change the CTS tests, partially because one can argue
that it actually is required, but also because of the CL-over-Vulkan
efforts, as well as DXVK and VKD3D which are emulating API's that need
comparisons with NaN to work correctly. Also, according to
https://patchwork.freedesktop.org/patch/206486/, apparently
Wolfenstein 2 actually does care about it and breaks if you change
ordered to unordered -- again, we're getting it right by dumb luck.
And it's probably likely that some DX game does it, and we also get it
right by dumb luck. Just think about how much crazy stuff games come
to rely on by accident! We could make comparisons precise in SPIR-V,
but then we'd need to make unordered comparisons fast anyways, and it
seems silly to let GL and SPIR-V diverge like that, especially when
Wine has been emulating DX over GL for a long time.

Best,

Connor

>
> >> On Thu, Nov 29, 2018 at 9:18 AM Samuel Pitoiset <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>
> >>> ---
> >>>  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
> >>> 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
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>