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

[Marek Olšák]

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