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

Wed Jan 11 21:27:28 UTC 2017

On Wed, Jan 11, 2017 at 1:12 PM, Erik Faye-Lund <kusmabite at gmail.com> wrote:

> On Wed, Jan 11, 2017 at 9:49 PM, Erik Faye-Lund <kusmabite at gmail.com>
> wrote:
> > On Wed, Jan 11, 2017 at 9:42 PM, Erik Faye-Lund <kusmabite at gmail.com>
> wrote:
> >> On Wed, Jan 11, 2017 at 9:22 PM, Samuel Pitoiset
> >> <samuel.pitoiset at gmail.com> wrote:
> >>>
> >>>
> >>> 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...
> >>
> >> Could be. But negative inputs to inversesqrt() returns NaN for me when
> >> writing a GLSL-shader in Render Monkey.
> >>
> >> This fragment shader produces red output:
> >>
> >> ---8<---
> >> void main(void)
> >> {
> >>     gl_FragColor = vec4(0.0);
> >>     float tmp = inversesqrt(-1.0);
> >>     if (tmp != tmp)
> >>        gl_FragColor.x = 1.0;
> >> }
> >> ---8<---
> >> And just to be sure, I've verified that passing the -1.0 through a
> >> varying doesn't change the result, neither does negative non-constant
> >> values.
> >>
> >> Now I've even tested on two different NVIDIA-GPUs, with two different
> >> driver versions, both give the same result. I even tried on Intel's
> >> Windows drivers, same result. I'm starting to think that this issue
> >> hasn't been diagnosed correctly.
> >
> > Actually, the test on the Intel Windows-driver was a mistake (optimus
> > mixup); Intel seems to do abs on Windows.
>
> Actually, the Intel Windows driver is even more confusing;
> inversesqrt(-1.0) from a constant does not produce NaN, but from a
> varying it does! So yeah, that driver seems to have its issues.
>

I think this is a good argument for doing the workaround at the GLSL IR
level if we choose to do it.  GLSL IR, NIR, LLVM, they all constant-fold.
If we want constant folding to match what happens when it hits hardwawre,
we need to put the abs() in at the highest level.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170111/36320eb8/attachment-0001.html>