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

[Jason Ekstrand]

On Tue, Jan 10, 2017 at 9:48 AM, 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).
>

As a quick addendum, this won't actually hurt our performance (at least not
on modern hardware) because we can do source modifiers on SQRT and RSQ.


> On Tue, Jan 10, 2017 at 1:24 AM, Samuel Pitoiset <
> samuel.pitoiset at gmail.com> wrote:
>
>>
>>
>> On 01/09/2017 10:03 PM, Roland Scheidegger wrote:
>>
>>> Am 06.01.2017 um 10:42 schrieb Samuel Pitoiset:
>>>
>>>> D3D always computes the absolute value while GLSL says that the
>>>>
>>> That should probably say "d3d9" - it is completely wrong for d3d10 and
>>> later (which have it to be defined as a guaranteed NaN).
>>> (Otherwise, I'm still not quite convinced it's the right thing to do
>>> this always, but meh...)
>>>
>>
>> I'm not familiar with D3D, thanks for noticing. Fixed locally.
>>
>>
>>
>>> Roland
>>>
>>>
>>> result of inversesqrt() is undefined if x <= 0 (and undefined if
>>>> x < 0 for sqrt()). But some apps rely on this specific behaviour
>>>> which is not clearly defined by OpenGL.
>>>>
>>>> Computing the absolute value before sqrt()/inversesqrt() will
>>>> prevent that, especially for apps which have been ported from D3D.
>>>> Note that closed drivers seem to also use that quirk.
>>>>
>>>> This gets rid of the NaN values in the "Spec Ops: The Line" game
>>>> as well as the black squares with radeonsi. Note that Nouveau is
>>>> not affected by this bug because we already take the absolute value
>>>> when translating from TGSI to nv50/ir.
>>>>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97338
>>>>
>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>>> ---
>>>>  src/compiler/glsl/builtin_functions.cpp | 22 ++++++++++++++++++++--
>>>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/compiler/glsl/builtin_functions.cpp
>>>> b/src/compiler/glsl/builtin_functions.cpp
>>>> index 797af08b6c..f816f2ff7d 100644
>>>> --- a/src/compiler/glsl/builtin_functions.cpp
>>>> +++ b/src/compiler/glsl/builtin_functions.cpp
>>>> @@ -3623,12 +3623,30 @@ builtin_builder::_pow(const glsl_type *type)
>>>>     return binop(always_available, ir_binop_pow, type, type, type);
>>>>  }
>>>>
>>>> +ir_function_signature *
>>>> +builtin_builder::_sqrt(builtin_available_predicate avail,
>>>> +                       const glsl_type *type)
>>>> +{
>>>> +   ir_variable *x = in_var(type, "x");
>>>> +   MAKE_SIG(type, avail, 1, x);
>>>> +   body.emit(ret(expr(ir_unop_sqrt, abs(x))));
>>>> +   return sig;
>>>> +}
>>>> +
>>>> +ir_function_signature *
>>>> +builtin_builder::_inversesqrt(builtin_available_predicate avail,
>>>> +                              const glsl_type *type)
>>>> +{
>>>> +   ir_variable *x = in_var(type, "x");
>>>> +   MAKE_SIG(type, avail, 1, x);
>>>> +   body.emit(ret(expr(ir_unop_rsq, abs(x))));
>>>> +   return sig;
>>>> +}
>>>> +
>>>>  UNOP(exp,         ir_unop_exp,  always_available)
>>>>  UNOP(log,         ir_unop_log,  always_available)
>>>>  UNOP(exp2,        ir_unop_exp2, always_available)
>>>>  UNOP(log2,        ir_unop_log2, always_available)
>>>> -UNOPA(sqrt,        ir_unop_sqrt)
>>>> -UNOPA(inversesqrt, ir_unop_rsq)
>>>>
>>>>  /** @} */
>>>>
>>>>
>>>>
>>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170110/8a248422/attachment-0001.html>