[Mesa-dev] [PATCH 1/2] nir: add a compiler option for disabling float comparison simplifications
Ian Romanick
idr at freedesktop.org
Mon Dec 3 19:34:02 UTC 2018
On 12/01/2018 05:58 AM, Connor Abbott wrote:
> 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.
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 <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
>>>
>>
>
More information about the mesa-dev
mailing list