[Mesa-dev] [PATCH 1/2] nir: add a compiler option for disabling float comparison simplifications
Jason Ekstrand
jason at jlekstrand.net
Fri Nov 30 21:39:40 UTC 2018
On Fri, Nov 30, 2018 at 3:37 PM Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
wrote:
> On Fri, Nov 30, 2018 at 10:29 PM Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> >
> > 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.
>
> What do you mean an extension? AFAIU the concern is that Vulkan SPIR-V
> is more restrictive than GLSL here, and disallows these optimization
> right? That makes a strong case that we should remove these rules for
> at least Vulkan. If that means writing a CTS test, maybe we should do
> just that?
>
I know there were some discussions around this. I don't remember what the
outcome was. It's possible that it really was "Vulkan is just more
restrictive" but I don't remember. I know there are CTS tests for these
opcodes and, last I checked, we're passing the CTS so maybe those tests
aren't mustpass? I'd have to go digging.
--Jason
> >
> >>
> >> >> 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
> >> >
> >>
> > _______________________________________________
> > 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/526ea6a1/attachment-0001.html>
More information about the mesa-dev
mailing list