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

[Marek Olšák]

On Wed, Jan 11, 2017 at 7:34 PM, Erik Faye-Lund <kusmabite at gmail.com> 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.

I'm open to a drirc-based solution, but I'd also be OK with abs in
radeonsi or GLSL. It's not like nouveau and r600g are gonna remove the
ABS modifier, and I don't know if people play such demanding games on
i965, so maybe this is a radeonsi issue only.

Marek