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

[Marek Olšák]

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. I've heard rumors (also thanks to an ex-nvidia
employee) that the Direct3D world is much worse.

Marek