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 <cwabbo...@gmail.com> wrote: > On Sat, Jan 3, 2015 at 2:18 PM, Thomas Helland > <thomashellan...@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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
