[Mesa-dev] [PATCH 05/22] glsl: Add sqrt, rsq, exp, exp2 to get_range

[Marek Olšák]

Also, you seem to be assuming that IEEE and D3D10 versions of the
min() and max() are the same, which is not true. Both GLSL and D3D10
versions should work according to IEEE754, yet they are different. The
GLSL versions use conditional assignments, which is the same as C. The
D3D10 versions are defined in IEEE as "minNum" and "maxNum" and have
different behavior with regard to NaNs (cannot be implemented as one
conditional assignment).

Marek

On Sun, Jan 11, 2015 at 12:57 PM, Marek Olšák <maraeo at gmail.com> wrote:
> Well, max(x, 0) has no effect on NaNs. According to the GLSL
> specification, max() should return x if either argument is NaN. So the
> correct way to convert NaN to 0 is:
>
> max(0, y)
>
> That implies that min and max aren't commutative if NaNs are supported.
>
> Marek
>
> On Fri, Jan 9, 2015 at 4:15 AM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>> On Sat, Jan 3, 2015 at 2:18 PM, Thomas Helland
>> <thomashelland90 at gmail.com> wrote:
>>> Also handle undefined behaviour for sqrt(x) where x < 0
>>> and rsq(x) where x <= 0.
>>>
>>> This gives us some reduction in instruction count on three
>>> Dungeon Defenders shaders as they are doing: max(exp(x), 0)
>>
>> So initially when you said that Dungeon Defenders was doing
>> max(exp(x), 0), my thought was "wat?" but after thinking about it some
>> more, I can see why it would do this. The GLSL spec doesn't guarantee
>> that implementations of +, *, exp(), etc. will return NaN when one of
>> the arguments is NaN, but it also doesn't guarantee that they *won't*;
>> in other words, if for some strange reason you need the old-style
>> never-return-NaN functionality, you need to do something like what
>> this game is doing. For implementations that don't return NaN, this
>> optimization is just fine, but if you remove it when the HW does
>> return NaN, then whatever's using the result might get a NaN when it's
>> not expecting it, leading to Bad Things happening. Maybe it isn't an
>> issue with this particular game, but in order to be correct here it
>> seems like we do have to take NaN's into account after all.
>>
>> There was a related thread (and other discussions) about the behavior
>> of min/max wrt NaN's:
>>
>> http://lists.freedesktop.org/archives/mesa-dev/2014-December/073182.html
>>
>> My conclusion is that basically everyone that actually produces NaN's
>> follows the IEEE/D3D behavior here, which I'm assuming the Dungeon
>> Defenders developers were probably depending on.
>>
>>>
>>> v2: Change to use new IS_CONSTANT() macro
>>>     Fix high unintenionally not being returned
>>>     Add some air for readability
>>>     Comment on the exploit of undefined behavior
>>>     Constify mem_ctx
>>> ---
>>>  src/glsl/opt_minmax.cpp | 31 +++++++++++++++++++++++++++++++
>>>  1 file changed, 31 insertions(+)
>>>
>>> diff --git a/src/glsl/opt_minmax.cpp b/src/glsl/opt_minmax.cpp
>>> index 56805c0..2faa3c3 100644
>>> --- a/src/glsl/opt_minmax.cpp
>>> +++ b/src/glsl/opt_minmax.cpp
>>> @@ -274,9 +274,40 @@ get_range(ir_rvalue *rval)
>>>     minmax_range r0;
>>>     minmax_range r1;
>>>
>>> +   void *const mem_ctx = ralloc_parent(rval);
>>> +
>>> +   ir_constant *low = NULL;
>>> +   ir_constant *high = NULL;
>>> +
>>>     if (expr) {
>>>        switch (expr->operation) {
>>>
>>> +      case ir_unop_exp:
>>> +      case ir_unop_exp2:
>>> +      case ir_unop_sqrt:
>>> +      case ir_unop_rsq:
>>> +         r0 = get_range(expr->operands[0]);
>>> +
>>> +         /* The spec says sqrt is undefined if x < 0
>>> +          * We can use this to set the range to whatever we want
>>> +          */
>>> +         if (expr->operation == ir_unop_sqrt &&
>>> +             IS_CONSTANT(r0.high, <, 0.0f))
>>> +            high = new(mem_ctx) ir_constant(0.0f);
>>> +
>>> +         /* The spec says rsq is undefined if x <= 0
>>> +          * We can use this to set the range to whatever we want
>>> +          */
>>> +         if (expr->operation == ir_unop_rsq &&
>>> +             IS_CONSTANT(r0.high, <=, 0.0f))
>>> +            high = new(mem_ctx) ir_constant(0.0f);
>>> +
>>> +         /* TODO: If we know, i.e, the lower range of the operand
>>> +          * we can calculate the lower range
>>> +          */
>>> +         low = new(mem_ctx) ir_constant(0.0f);
>>> +         return minmax_range(low, high);
>>> +
>>>        case ir_binop_min:
>>>        case ir_binop_max:
>>>           r0 = get_range(expr->operands[0]);
>>> --
>>> 2.2.1
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev