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

[Erik Faye-Lund]

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.