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

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

2018-12-03 Thread Ian Romanick

On 11/29/2018 07:47 AM, Connor Abbott wrote:
> On Thu, Nov 29, 2018 at 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".
> 
> 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
> 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

I think this is the real problem.  There's this block in
nir_opt_algebraic:

# 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)),
   ]

The assumption is that dropping the 'a == a' will not affect the result,
but if the whole tree was actually '!(a == a && a >= b)' dropping the 'a
== a' does affect the result.  At the very least, I think this should be
modified (somehow?) to mark the replacement expression as precise.  If
we don't already, we should also make sure the isnan function marks the
comparison precise.

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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-12-03 Thread Ian Romanick

On 12/01/2018 05:58 AM, Connor Abbott wrote:
> On Fri, Nov 30, 2018 at 10:18 PM Ian Romanick  wrote:
>>
>> On 11/29/2018 07:47 AM, Connor Abbott wrote:
>>> On Thu, Nov 29, 2018 at 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".
>>>
>>> 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.

GLSL 4.6 says:

(In section 4.1.4 Floating-Point Variables) "While encodings are
logically IEEE 754, operations (addition, multiplication, etc.) are not
necessarily performed as required by IEEE 754. See section 4.7.1 “Range
and Precision” for more details on precision and usage of NaNs (Not a
Number) and Infs (positive or negative infinities)."

(In section 4.7.1 Range and Precision) "NaNs are not required to be
generated.  Support for signaling NaNs is not required and exceptions
are never raised. Operations and built-in functions that operate on a
NaN are not required to return a NaN as the result."

(In the description of intBitsToFloat) "If a NaN is passed in, it will
not signal, and the resulting value is unspecified."

(In the description of packDouble2x32) "If an IEEE 754 Inf or NaN is
created, it will not signal, and the resulting floating-point value is
unspecified."

Aside from the description of the isnan function, that is all the
mention of NaN in the spec.  If you are depending on any particular NaN
behavior, you have already lost.  I will emphasize "operations... are
not necessarily performed as required by IEEE 754."

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

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

2018-12-01 Thread Samuel Pitoiset




On 12/1/18 3:36 PM, Connor Abbott wrote:

On Sat, Dec 1, 2018 at 3:22 PM Samuel Pitoiset
 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
mailto:samuel.pitoi...@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 mailto:samuel.pitoi...@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@lists.freedesktop.org 
 https://lists.freedesktop.org/mailman/listinfo/mesa-dev


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-12-01 Thread Connor Abbott

On Sat, Dec 1, 2018 at 3:22 PM Samuel Pitoiset
 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.

>
> 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
> > mailto:samuel.pitoi...@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  > >
> > ---
> >   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@lists.freedesktop.org 
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-12-01 Thread Samuel Pitoiset

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!

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 
mailto:samuel.pitoi...@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 mailto:samuel.pitoi...@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@lists.freedesktop.org 
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-12-01 Thread Connor Abbott

On Fri, Nov 30, 2018 at 10:18 PM Ian Romanick  wrote:
>
> On 11/29/2018 07:47 AM, Connor Abbott wrote:
> > On Thu, Nov 29, 2018 at 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".
> >
> > 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 
> >>  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 
> >>> ---
> >>>  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 = [
> >>>

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

2018-11-30 Thread Ian Romanick

On 11/30/2018 02:36 PM, Jason Ekstrand wrote:
> On Fri, Nov 30, 2018 at 4:34 PM Ian Romanick  > wrote:
> 
> On 11/30/2018 01:29 PM, Jason Ekstrand wrote:
> > On Fri, Nov 30, 2018 at 3:18 PM Ian Romanick  
> > >> wrote:
> >
> >     On 11/29/2018 07:47 AM, Connor Abbott wrote:
> >     > On Thu, Nov 29, 2018 at 4:22 PM Jason Ekstrand
> >     mailto:ja...@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.
> 
> Why isn't it sufficient to mark the operation as precise?  Was that on
> IRC, and I missed it?
> 
> It is also possible that we could improve the handling of 'if
> (!condition)' in the backend to make these transformations less
> important.  I have some patches for that somewhere.  They didn't really
> help with these transformations in place, and I never measured the
> result without these transformations.
> 
> I think we can and that would be better than this flag.

Ok.  I'll resurrect my patches next week.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-11-30 Thread Ian Romanick

On 11/30/2018 01:36 PM, Bas Nieuwenhuizen wrote:
> On Fri, Nov 30, 2018 at 10:29 PM Jason Ekstrand  wrote:
>>
>> On Fri, Nov 30, 2018 at 3:18 PM Ian Romanick  wrote:
>>>
>>> On 11/29/2018 07:47 AM, Connor Abbott wrote:
 On Thu, Nov 29, 2018 at 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".

 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?

Given the existence of mobile GPUs, it seems unlikely to me that Vulkan
SPIR-V is more strict about NaNs or denorms than GLSL or GLSL ES.  The
mobile vendors didn't have the extra DX requirements, so, for a variety
of valid reasons, they pushed back on making much of anything more strict.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-11-30 Thread Jason Ekstrand

On Fri, Nov 30, 2018 at 4:34 PM Ian Romanick  wrote:

> On 11/30/2018 01:29 PM, Jason Ekstrand wrote:
> > On Fri, Nov 30, 2018 at 3:18 PM Ian Romanick  > > wrote:
> >
> > On 11/29/2018 07:47 AM, Connor Abbott wrote:
> > > On Thu, Nov 29, 2018 at 4:22 PM Jason Ekstrand
> > mailto:ja...@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.
>
> Why isn't it sufficient to mark the operation as precise?  Was that on
> IRC, and I missed it?
>
> It is also possible that we could improve the handling of 'if
> (!condition)' in the backend to make these transformations less
> important.  I have some patches for that somewhere.  They didn't really
> help with these transformations in place, and I never measured the
> result without these transformations.
>

I think we can and that would be better than this flag.

--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-11-30 Thread Ian Romanick

On 11/30/2018 01:29 PM, Jason Ekstrand wrote:
> On Fri, Nov 30, 2018 at 3:18 PM Ian Romanick  > wrote:
> 
> On 11/29/2018 07:47 AM, Connor Abbott wrote:
> > On Thu, Nov 29, 2018 at 4:22 PM Jason Ekstrand
> mailto:ja...@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.

Why isn't it sufficient to mark the operation as precise?  Was that on
IRC, and I missed it?

It is also possible that we could improve the handling of 'if
(!condition)' in the backend to make these transformations less
important.  I have some patches for that somewhere.  They didn't really
help with these transformations in place, and I never measured the
result without these transformations.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-11-30 Thread Jason Ekstrand

On Fri, Nov 30, 2018 at 3:37 PM Bas Nieuwenhuizen 
wrote:

> On Fri, Nov 30, 2018 at 10:29 PM Jason Ekstrand 
> wrote:
> >
> > On Fri, Nov 30, 2018 at 3:18 PM Ian Romanick 
> wrote:
> >>
> >> On 11/29/2018 07:47 AM, Connor Abbott wrote:
> >> > On Thu, Nov 29, 2018 at 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".
> >> >
> >> > 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.pitoi...@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 
> >> >>> ---
> >> >>>  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),
>

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

2018-11-30 Thread Bas Nieuwenhuizen

On Fri, Nov 30, 2018 at 10:29 PM Jason Ekstrand  wrote:
>
> On Fri, Nov 30, 2018 at 3:18 PM Ian Romanick  wrote:
>>
>> On 11/29/2018 07:47 AM, Connor Abbott wrote:
>> > On Thu, Nov 29, 2018 at 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".
>> >
>> > 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?


>
>>
>> >> On Thu, Nov 29, 2018 at 9:18 AM Samuel Pitoiset 
>> >>  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 
>> >>> ---
>> >>>  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@lists.freedesktop.org
>> >>>

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

2018-11-30 Thread Jason Ekstrand

On Fri, Nov 30, 2018 at 3:18 PM Ian Romanick  wrote:

> On 11/29/2018 07:47 AM, Connor Abbott wrote:
> > On Thu, Nov 29, 2018 at 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".
> >
> > 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.pitoi...@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 
> >>> ---
> >>>  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@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >>
> >> ___
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org

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

2018-11-30 Thread Ian Romanick

On 11/29/2018 07:47 AM, Connor Abbott wrote:
> On Thu, Nov 29, 2018 at 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".
> 
> 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.

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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-11-29 Thread Timothy Arceri

On Thu, Nov 29, 2018, at 7:47 AM, Connor Abbott wrote:
> On Thu, Nov 29, 2018 at 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".
> 
> 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
> 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.

FYI here are the shader-db results (SKL) from removing them:

total instructions in shared programs: 12858124 -> 12889104 (0.24%)
instructions in affected programs: 1687380 -> 1718360 (1.84%)
helped: 2
HURT: 7073

total cycles in shared programs: 317838109 -> 318266406 (0.13%)
cycles in affected programs: 62285268 -> 62713565 (0.69%)
helped: 1017
HURT: 6552

total loops in shared programs: 3808 -> 3808 (0.00%)
loops in affected programs: 0 -> 0
helped: 0
HURT: 0

total spills in shared programs: 6793 -> 6785 (-0.12%)
spills in affected programs: 166 -> 158 (-4.82%)
helped: 2
HURT: 0

total fills in shared programs: 9561 -> 9541 (-0.21%)
fills in affected programs: 852 -> 832 (-2.35%)
helped: 2
HURT: 3

LOST:   0
GAINED: 1 

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

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

2018-11-29 Thread Connor Abbott

On Thu, Nov 29, 2018 at 4:47 PM Connor Abbott  wrote:
>
> On Thu, Nov 29, 2018 at 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".
>
> 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
> 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.

Sorry... by SPIR-V, I meant Vulkan SPIR-V. Specifically Appendix A of
the Vulkan spec, in the "Precision and Operation of SPIR-V
Instructions" section. There's a section in the GLSL 4.50 spec which
is identical AFAICT.

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

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

2018-11-29 Thread Connor Abbott

On Thu, Nov 29, 2018 at 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".

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

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

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

2018-11-29 Thread Jason Ekstrand

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 
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 
> ---
>  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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

18 matches

Site Navigation

Mail list logo

Footer information