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

[Jason Ekstrand]

On Fri, Nov 30, 2018 at 3: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.
>
> > 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.
>

The test case came from VKD3D which does D3D12 on Vulkan.  Someone (Samuel,
maybe?) was going to ask around and see if we can figure out what D3D12's
rules are.  It's possible that it requires IEEE or something close.  If
that's the case, as I said to Samuel on IRC, we're probably looking at an
extension.  I don't think we want a flag like this that's set per-API.


> >> 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
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181130/f6eaf968/attachment.html>