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

[Jason Ekstrand]

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).

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/e89b555a/attachment.html>