[Mesa-dev] [PATCH] glsl: always do sqrt(abs()) and inversesqrt(abs())

[Erik Faye-Lund]

On Wed, Jan 11, 2017 at 9:49 PM, Erik Faye-Lund <kusmabite at gmail.com> wrote:
> On Wed, Jan 11, 2017 at 9:42 PM, Erik Faye-Lund <kusmabite at gmail.com> wrote:
>> On Wed, Jan 11, 2017 at 9:22 PM, Samuel Pitoiset
>> <samuel.pitoiset at gmail.com> wrote:
>>>
>>>
>>> On 01/11/2017 07:34 PM, Erik Faye-Lund wrote:
>>>>
>>>> On Wed, Jan 11, 2017 at 7:33 PM, Jason Ekstrand <jason at jlekstrand.net>
>>>> wrote:
>>>>>
>>>>> On Wed, Jan 11, 2017 at 10:31 AM, Erik Faye-Lund <kusmabite at gmail.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On Wed, Jan 11, 2017 at 7:22 PM, Marek Olšák <maraeo at gmail.com> wrote:
>>>>>>>
>>>>>>> On Wed, Jan 11, 2017 at 7:09 PM, Jason Ekstrand <jason at jlekstrand.net>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On Wed, Jan 11, 2017 at 9:32 AM, Samuel Pitoiset
>>>>>>>> <samuel.pitoiset at gmail.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 01/11/2017 05:32 PM, Marek Olšák wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Wed, Jan 11, 2017 at 4:33 PM, Erik Faye-Lund
>>>>>>>>>> <kusmabite at gmail.com>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Jan 11, 2017 at 4:14 PM, Nicolai Hähnle
>>>>>>>>>>> <nhaehnle at gmail.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 11.01.2017 13:17, Marek Olšák wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Jan 10, 2017 at 6:48 PM, Jason Ekstrand
>>>>>>>>>>>>> <jason at jlekstrand.net>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'll be honest, I'm not a fan... Given that D3D10 has one
>>>>>>>>>>>>>> defined
>>>>>>>>>>>>>> behavior,
>>>>>>>>>>>>>> D3D9 has another, and GL doesn't specify, I don't really think
>>>>>>>>>>>>>> we
>>>>>>>>>>>>>> should
>>>>>>>>>>>>>> be
>>>>>>>>>>>>>> making a global change to all drivers to do the D3D9 behavior
>>>>>>>>>>>>>> just to
>>>>>>>>>>>>>> fix
>>>>>>>>>>>>>> one app.  Sure, other apps probably have the same bug, but are
>>>>>>>>>>>>>> we
>>>>>>>>>>>>>> going
>>>>>>>>>>>>>> to
>>>>>>>>>>>>>> have apps that expect the D3D10 behavior that we've now
>>>>>>>>>>>>>> explicitly
>>>>>>>>>>>>>> made
>>>>>>>>>>>>>> not
>>>>>>>>>>>>>> work?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> If we're going to hack around an app bug, I would really rather
>>>>>>>>>>>>>> see
>>>>>>>>>>>>>> it
>>>>>>>>>>>>>> behind a driconf option rather than a global change to driver
>>>>>>>>>>>>>> behavior.
>>>>>>>>>>>>>> Even better, it'd be cool if we could see the app get fixed.
>>>>>>>>>>>>>> (Yes, I
>>>>>>>>>>>>>> know
>>>>>>>>>>>>>> that's not likely).
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think we are not in a position to refuse this workaround, or
>>>>>>>>>>>>> put
>>>>>>>>>>>>> more precisely, to have a different behavior from everybody else.
>>>>>>>>>>>>> By
>>>>>>>>>>>>> "we", I mean i965, radeonsi, svga. All closed drivers use abs.
>>>>>>>>>>>>> Many
>>>>>>>>>>>>> Mesa drivers also use abs internally (r300, r600, nv30,
>>>>>>>>>>>>> nv50/nvc0).
>>>>>>>>>>>>> This is not really a workaround for a specific application, even
>>>>>>>>>>>>> though it's strongly motivated by that. It's a fix to align the
>>>>>>>>>>>>> few
>>>>>>>>>>>>> remaining drivers with all others.
>>>>>>>>>>>>>
>>>>>>>>>>>>> We talked with the publisher about this a very long time ago.
>>>>>>>>>>>>> While I
>>>>>>>>>>>>> don't remember the details (Nicolai?), I think they refused to
>>>>>>>>>>>>> fix
>>>>>>>>>>>>> it
>>>>>>>>>>>>> because radeonsi appeared to be the only driver not doing abs.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> If I remember correctly, it wasn't so much a refusal as a lack of
>>>>>>>>>>>> follow-through. They even had an option in their framework to add
>>>>>>>>>>>> the
>>>>>>>>>>>> abs(...) when translating shaders, but somehow didn't turn it on
>>>>>>>>>>>> unconditionally for some reason...
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> VP even says so here:
>>>>>>>>>>> https://github.com/virtual-programming/specops-linux/issues/20
>>>>>>>>>>>
>>>>>>>>>>> They recommend against patching mesa to do abs, though.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> We should still patch Mesa to align the behavior with closed drivers
>>>>>>>>>> and gallium drivers like r600g and nouveau. In other words, it's too
>>>>>>>>>> late to tell us not to patch Mesa, because r600g and nouveau have
>>>>>>>>>> been
>>>>>>>>>> "patched" since the beginning.
>>>>>>>>>>
>>>>>>>>>> We only need to decide whether we should do it in the GLSL compiler
>>>>>>>>>> or
>>>>>>>>>> radeonsi, i.e. whether we should exclude i965 and svga.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I do agree with that.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I tend to disagree but I've come to the conclusion that I won't stand
>>>>>>>> in the
>>>>>>>> way either.  If both of the other desktop vendors do it and we've
>>>>>>>> already
>>>>>>>> decided that no implementation we care about will have its performance
>>>>>>>> impacted, it seems like a valid spec-compliant thing to do.  I would
>>>>>>>> prefer
>>>>>>>> it to be behind a driconf option, but if it's unconditional, oh well.
>>>>>>>> My
>>>>>>>> disagreement is mostly philosophical.
>>>>>>>>
>>>>>>>> Over the last two years of working on Vulkan, I've been fighting
>>>>>>>> broken
>>>>>>>> tests and apps left and right.  Vulkan has a huge amount of area
>>>>>>>> where,
>>>>>>>> if
>>>>>>>> an app does something wrong, they get undefined behavior which is up
>>>>>>>> to
>>>>>>>> and
>>>>>>>> including program termination.  And basically all apps are broken in
>>>>>>>> some
>>>>>>>> way.  Fortunately, the validation layers are finally starting to catch
>>>>>>>> up to
>>>>>>>> the point where I'm noticing very few bugs that the validation layers
>>>>>>>> don't
>>>>>>>> catch and things are getting into a better state.  However, I've had
>>>>>>>> more
>>>>>>>> discussions than I can count with people where I have to explain to
>>>>>>>> them
>>>>>>>> that "No, the app is broken.  It needs to be fixed.  It's not my job
>>>>>>>> to
>>>>>>>> make
>>>>>>>> it work."  Once you start allowing brokenness, you can never stop
>>>>>>>> allowing
>>>>>>>> it and you paint yourself into a corner.  Suddenly, you go to make a
>>>>>>>> change,
>>>>>>>> and your design decisions are not guided by the spec, they're guided
>>>>>>>> by
>>>>>>>> the
>>>>>>>> spec *and* all of the broken apps that you have to keep working on
>>>>>>>> your
>>>>>>>> driver because you let something through.
>>>>>>>>
>>>>>>>> In the world of GLES and OpenGL conformance, we fight the same fight.
>>>>>>>> When
>>>>>>>> people ask me how conformance is coming, I frequently answer with,
>>>>>>>> "We've
>>>>>>>> got a bunch of people fixing <insert test suite name here> so that our
>>>>>>>> driver passes".  It's not that mesa is particularly touchy, it's that
>>>>>>>> a
>>>>>>>> good
>>>>>>>> chunk of the rest of the industry just hacks around everything inside
>>>>>>>> their
>>>>>>>> driver and doesn't bother to fix the tests.  Sometimes the driver that
>>>>>>>> passes the conformance suite isn't even the one they ship.  If we're
>>>>>>>> going
>>>>>>>> to have a spec and hardware vendors (or the FOSS community) are going
>>>>>>>> to
>>>>>>>> implement it and apps are going to write to it, then we all need to
>>>>>>>> agree on
>>>>>>>> what it means and play by the rules.  If an app doesn't play by the
>>>>>>>> rules
>>>>>>>> and does something with undefined behavior, then it's a broken app.
>>>>>>>> If
>>>>>>>> we
>>>>>>>> say, "No, it's ok, you don't have to fix it.  We'll just hack around
>>>>>>>> it"
>>>>>>>> we're enablers for their broken behavior and the broken behavior
>>>>>>>> continues.
>>>>>>>> In this particular case, we're dealing with a broken app.  The only
>>>>>>>> real
>>>>>>>> issue is that all of the drivers that point out the issues were not
>>>>>>>> drivers
>>>>>>>> they tested on.
>>>>>>>>
>>>>>>>> Another reason why I'm not a huge fan is that there is some momentum
>>>>>>>> in
>>>>>>>> the
>>>>>>>> industry to make GLSL better defined with respect to NaN.  I don't
>>>>>>>> know
>>>>>>>> that
>>>>>>>> anything will ever come of it (because it may break apps) but if
>>>>>>>> something
>>>>>>>> does, we may find ourselves having to make SQRT and RSQ NaN-correct in
>>>>>>>> the
>>>>>>>> future and, hey look, it'll break apps.
>>>>>>>>
>>>>>>>> Ok, rant over.  Push it if you want.  You can even put my nakked-by on
>>>>>>>> it if
>>>>>>>> you'd like. :-)
>>>>>>>
>>>>>>>
>>>>>>> I agree with you completely, and I find it unfortunate too that we
>>>>>>> have to add the workaround to GLSL or radeonsi to align its behavior
>>>>>>> with closed drivers.
>>>>>>
>>>>>>
>>>>>> Just for reference, I just tested what NVIDIA does on Windows, and
>>>>>> they *don't* seem to do inversesqrt(abs(x)) on my HW/driver.
>>>>>
>>>>>
>>>>>
>>>>> What about sqrt()?  Do they do abs for one and not the other?  Because
>>>>> that
>>>>> would be crazy but also possible.
>>>>
>>>>
>>>> Not for sqrt either, it seems.
>>>
>>>
>>> Huh? I'm sure I have seen NVIDIA doing rsq(abs()) one day. Maybe it was
>>> specific to that application but I don't remember the name...
>>
>> Could be. But negative inputs to inversesqrt() returns NaN for me when
>> writing a GLSL-shader in Render Monkey.
>>
>> This fragment shader produces red output:
>>
>> ---8<---
>> void main(void)
>> {
>>     gl_FragColor = vec4(0.0);
>>     float tmp = inversesqrt(-1.0);
>>     if (tmp != tmp)
>>        gl_FragColor.x = 1.0;
>> }
>> ---8<---
>> And just to be sure, I've verified that passing the -1.0 through a
>> varying doesn't change the result, neither does negative non-constant
>> values.
>>
>> Now I've even tested on two different NVIDIA-GPUs, with two different
>> driver versions, both give the same result. I even tried on Intel's
>> Windows drivers, same result. I'm starting to think that this issue
>> hasn't been diagnosed correctly.
>
> Actually, the test on the Intel Windows-driver was a mistake (optimus
> mixup); Intel seems to do abs on Windows.

Actually, the Intel Windows driver is even more confusing;
inversesqrt(-1.0) from a constant does not produce NaN, but from a
varying it does! So yeah, that driver seems to have its issues.