On Wed, Jan 11, 2017 at 1:12 PM, Erik Faye-Lund <kusmab...@gmail.com> wrote:
> On Wed, Jan 11, 2017 at 9:49 PM, Erik Faye-Lund <kusmab...@gmail.com> > wrote: > > On Wed, Jan 11, 2017 at 9:42 PM, Erik Faye-Lund <kusmab...@gmail.com> > wrote: > >> On Wed, Jan 11, 2017 at 9:22 PM, Samuel Pitoiset > >> <samuel.pitoi...@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 <ja...@jlekstrand.net > > > >>>> wrote: > >>>>> > >>>>> On Wed, Jan 11, 2017 at 10:31 AM, Erik Faye-Lund < > kusmab...@gmail.com> > >>>>> wrote: > >>>>>> > >>>>>> > >>>>>> On Wed, Jan 11, 2017 at 7:22 PM, Marek Olšák <mar...@gmail.com> > wrote: > >>>>>>> > >>>>>>> On Wed, Jan 11, 2017 at 7:09 PM, Jason Ekstrand < > ja...@jlekstrand.net> > >>>>>>> wrote: > >>>>>>>> > >>>>>>>> On Wed, Jan 11, 2017 at 9:32 AM, Samuel Pitoiset > >>>>>>>> <samuel.pitoi...@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 > >>>>>>>>>> <kusmab...@gmail.com> > >>>>>>>>>> wrote: > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> On Wed, Jan 11, 2017 at 4:14 PM, Nicolai Hähnle > >>>>>>>>>>> <nhaeh...@gmail.com> > >>>>>>>>>>> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> On 11.01.2017 13:17, Marek Olšák wrote: > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> On Tue, Jan 10, 2017 at 6:48 PM, Jason Ekstrand > >>>>>>>>>>>>> <ja...@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.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
