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

Wed Jan 11 20:22:10 UTC 2017

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

>