<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 11, 2017 at 1:12 PM, Erik Faye-Lund <<a href="mailto:kusmabite@gmail.com" target="_blank">kusmabite@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Jan 11, 2017 at 9:49 PM, Erik Faye-Lund <<a href="mailto:kusmabite@gmail.com">kusmabite@gmail.com</a>> wrote: > On Wed, Jan 11, 2017 at 9:42 PM, Erik Faye-Lund <<a href="mailto:kusmabite@gmail.com">kusmabite@gmail.com</a>> wrote: >> On Wed, Jan 11, 2017 at 9:22 PM, Samuel Pitoiset >> <<a href="mailto:samuel.pitoiset@gmail.com">samuel.pitoiset@gmail.com</a>> wrote: >>> >>> >>> On 01/11/2017 07:34 PM, Erik Faye-Lund wrote: >>>> >>>> On Wed, Jan 11, 2017 at 7:33 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> >>>> wrote: >>>>> >>>>> On Wed, Jan 11, 2017 at 10:31 AM, Erik Faye-Lund <<a href="mailto:kusmabite@gmail.com">kusmabite@gmail.com</a>> >>>>> wrote: >>>>>> >>>>>> >>>>>> On Wed, Jan 11, 2017 at 7:22 PM, Marek Olšák <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> wrote: >>>>>>> >>>>>>> On Wed, Jan 11, 2017 at 7:09 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> >>>>>>> wrote: >>>>>>>> >>>>>>>> On Wed, Jan 11, 2017 at 9:32 AM, Samuel Pitoiset >>>>>>>> <<a href="mailto:samuel.pitoiset@gmail.com">samuel.pitoiset@gmail.com</a>> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 01/11/2017 05:32 PM, Marek Olšák wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Jan 11, 2017 at 4:33 PM, Erik Faye-Lund >>>>>>>>>> <<a href="mailto:kusmabite@gmail.com">kusmabite@gmail.com</a>> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wed, Jan 11, 2017 at 4:14 PM, Nicolai Hähnle >>>>>>>>>>> <<a href="mailto:nhaehnle@gmail.com">nhaehnle@gmail.com</a>> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 11.01.2017 13:17, Marek Olšák wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Tue, Jan 10, 2017 at 6:48 PM, Jason Ekstrand >>>>>>>>>>>>> <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> >>>>>>>>>>>>> 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: >>>>>>>>>>> <a href="https://github.com/virtual-programming/specops-linux/issues/20" rel="noreferrer" target="_blank">https://github.com/virtual-programming/specops-linux/issues/20</a> >>>>>>>>>>> >>>>>>>>>>> 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. </div></div>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. </blockquote></div> </div><div class="gmail_extra">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. </div></div>