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

Wed Jan 11 18:09:26 UTC 2017

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. :-)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170111/16c4e271/attachment.html>