[Mesa-dev] [PATCH] radeonsi: compute the absolute value before RSQ

[Roland Scheidegger]

Am 06.01.2017 um 00:00 schrieb Marek Olšák:
> On Thu, Jan 5, 2017 at 10:06 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 05.01.2017 um 21:50 schrieb Samuel Pitoiset:
>>>
>>>
>>> On 01/05/2017 09:44 PM, Marek Olšák wrote:
>>>> On Thu, Jan 5, 2017 at 9:00 PM, Roland Scheidegger
>>>> <sroland at vmware.com> wrote:
>>>>> Am 05.01.2017 um 20:43 schrieb Samuel Pitoiset:
>>>>>>
>>>>>>
>>>>>> On 01/05/2017 06:49 PM, Roland Scheidegger wrote:
>>>>>>> Meh, I'm not really a big fan of such hacks. GPUs have support for
>>>>>>> NaNs
>>>>>>> since ages, and while glsl is lenient the point stands that
>>>>>>> returning a
>>>>>>> NaN is a more correct result, so doing extra work to get a wrong
>>>>>>> result
>>>>>>> doesn't look all that great to me.
>>>>>>> FWIW dx10 requires NaNs as results (for both sqrt and rsq). Maybe app
>>>>>>> specific quirks (if said apps can't be fixed) would be better...
>>>>>>> But well, it's your driver, so whatever floats your boat.
>>>>>>
>>>>>> I think the main idea here is to reduce the number of bug reports when
>>>>>> applications use D3D-style for RSQ/SQRT instead of what GLSL spec says.
>>>>>
>>>>> Well if someone ports from d3d10, he might well expect NaNs since d3d10
>>>>> guarantees you get a NaN there...
>>>>> (That said, with d3d10 NaNs will get converted to zeros on pixel shader
>>>>> export if the RT isn't a float type, I don't know what mesa drivers to
>>>>> there.)
>>>>> Not saying it wouldn't make sense doing such hackery at glsl level but
>>>>> imho it still should be a quirk.
>>>>
>>>> It looks like closed GL drivers always apply abs before rsq, so we
>>>> should do so too. The GLSL compiler seems like the correct place.
>>>
>>> IIRC, NVIDIA blob does it as well.
>> Ok, it's difficult to argue against that...
>> "Everybody is actually doing it wrong so we must follow them or we'll
>> see bugs..." Pretty sure this is essentially some issue coming from a
>> time when gpus didn't have NaNs so everybody relied on result being
>> non-NaN and now because all drivers do it still noone notices they still
>> have bugs relying on that behavior.
>> Anyway, I'm fine with any solution as long as the abs behavior isn't
>> implied by the tgsi opcode...
> 
> No, drivers are not doing it wrong. OpenGL says that sqrt(x < 0) is
> undefined, so drivers can do anything and it would be correct.
>
> It's only unfortunate that some apps rely on a specific behavior where
> no behavior is formally defined by OpenGL.
>

That's not really what I meant with "wrong".
GL says it's undefined, so yes doing an abs is correct. GL however
typically says things are undefined to not impose any specific behavior
(so no workarounds are needed for hw which might do something slightly
different for cases no one would care).
So the end result of that lenient spec is now exactly the opposite what
was intended and why I'm saying doing it is "wrong": you now do
workarounds in the driver even though the spec was crafted specifically
in a way that they should never be necessary even if your hw has weird
(wrt NaN) behavior (even disregarding that all hw, at least on the
desktop, will do proper ieee754 arithmetic nowadays).


> It looks like you don't like it because VMWare has GL<->D3D
> translators, but I don't see how this would impact VMWare, because if
> you replace Mesa with a closed driver, you'll have the same problem.

Yes, that's right, glsl can indeed be quite a pain due to poorly defined
behavior (e.g. for proper sqrt behavior you'd have to check for negative
values and replace with a NaN if that was the case, but in general it
won't work anyway due to poorly defined NaN behavior - fortunately only
conformance tests usually care about such things...).
But the reason I don't like it is really just that I don't like the idea
of doing extra work (yes I'm quite aware abs are cheap or maybe even
free on some hw) for nothing. Drivers shouldn't encourage broken app
behavior, albeit I suppose in that case that boat has sailed...

Roland