Re: [Mesa-dev] [v2 PATCH 09/16] glsl: Optimize clamp(x, 0, 1) as saturate(x)

2014-07-08 Thread Erik Faye-Lund 
On Mon, Jul 7, 2014 at 7:18 PM, Matt Turner matts...@gmail.com wrote:
 On Mon, Jul 7, 2014 at 6:57 AM, Abdiel Janulgue
 abdiel.janul...@linux.intel.com wrote:
 v2: Check that the base type is float (Ian Romanick)

 Signed-off-by: Abdiel Janulgue abdiel.janul...@linux.intel.com
 ---
  src/glsl/opt_algebraic.cpp | 34 ++
  1 file changed, 34 insertions(+)

 diff --git a/src/glsl/opt_algebraic.cpp b/src/glsl/opt_algebraic.cpp
 index ac7514a..2ad561c 100644
 --- a/src/glsl/opt_algebraic.cpp
 +++ b/src/glsl/opt_algebraic.cpp
 @@ -614,6 +614,40 @@ ir_algebraic_visitor::handle_expression(ir_expression 
 *ir)

break;

 +   case ir_binop_min:
 +   case ir_binop_max:
 +  if (ir-type-base_type != GLSL_TYPE_FLOAT)
 + break;
 +  /* Replace min(max) operations and its commutative combinations with
 +   * a saturate operation
 +   */
 +  for (int op = 0; op  2; op++) {
 + ir_expression *minmax = op_expr[op];
 + ir_constant *outer_const = op_const[(op == 0) ? 1 : 0];

 We use (1 - op) in other places in opt_algebraic. (Same comment below)

 + ir_expression_operation op_cond = (ir-operation == ir_binop_max) ?
 +ir_binop_min : ir_binop_max;
 +
 + if (!minmax || !outer_const || (minmax-operation != op_cond))
 +continue;
 +
 + /* Found a min/max operation. Now try to see if its operands
 +  * meet our conditions that we can do just a single saturate 
 operation
 +  */
 + for (int minmax_op = 0; minmax_op  2; minmax_op++) {
 +ir_rvalue *inner_val_a = minmax-operands[minmax_op];
 +ir_rvalue *inner_val_b = minmax-operands[(minmax_op == 0) ? 1 
 : 0];
 +
 +if (!inner_val_a || !inner_val_b)
 +   continue;
 +
 +/* Found a min (max(x, 0), 1.0) */

 This comment tripped me up for a second. This really means that you've
 found either

   - min(max(x, 0.0), 1.0); or
   - max(min(x, 1.0), 0.0)

Hmm, but are optimizing both of these to saturate OK? Shouldn't
min(max(NaN, 0.0), 1.0) give 0.0, whereas max(min(NaN, 1.0), 0.0) give
1.0?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [v2 PATCH 09/16] glsl: Optimize clamp(x, 0, 1) as saturate(x)

2014-07-08 Thread Matt Turner 
On Mon, Jul 7, 2014 at 11:00 PM, Erik Faye-Lund kusmab...@gmail.com wrote:
 On Mon, Jul 7, 2014 at 7:18 PM, Matt Turner matts...@gmail.com wrote:
 This comment tripped me up for a second. This really means that you've
 found either

   - min(max(x, 0.0), 1.0); or
   - max(min(x, 1.0), 0.0)

 Hmm, but are optimizing both of these to saturate OK? Shouldn't
 min(max(NaN, 0.0), 1.0) give 0.0, whereas max(min(NaN, 1.0), 0.0) give
 1.0?

Under standard IEEE rules, wouldn't the NaN propagate through all of
these expressions?

The GLSL 4.40 spec says

Operations and built-in functions that operate on a NaN are not required to
 return a NaN as the result.

So it seems like we have a lot of flexibility here. Is there some text
I'm missing?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [v2 PATCH 09/16] glsl: Optimize clamp(x, 0, 1) as saturate(x)

2014-07-08 Thread Erik Faye-Lund 
On Tue, Jul 8, 2014 at 8:22 AM, Matt Turner matts...@gmail.com wrote:
 On Mon, Jul 7, 2014 at 11:00 PM, Erik Faye-Lund kusmab...@gmail.com wrote:
 On Mon, Jul 7, 2014 at 7:18 PM, Matt Turner matts...@gmail.com wrote:
 This comment tripped me up for a second. This really means that you've
 found either

   - min(max(x, 0.0), 1.0); or
   - max(min(x, 1.0), 0.0)

 Hmm, but are optimizing both of these to saturate OK? Shouldn't
 min(max(NaN, 0.0), 1.0) give 0.0, whereas max(min(NaN, 1.0), 0.0) give
 1.0?

 Under standard IEEE rules, wouldn't the NaN propagate through all of
 these expressions?

 The GLSL 4.40 spec says

 Operations and built-in functions that operate on a NaN are not required to
  return a NaN as the result.

 So it seems like we have a lot of flexibility here. Is there some text
 I'm missing?

I think the point about flexibility is a bit weak or even
misunderstood: surely this applies to the built-ins used, not what we
generate after optimizing. So if we chose to do that, we'd need to
prevent min and max from propagating NaN even when issued stand-alone,
which might negatively impact performance in some ISAs.

As to why I reacted, I was just remembering that I read somewhere (one
of Humus' low-level shader optimization paper, IIRC) that the HLSL
compiler refused to do some similar optimizations for NaN-reasons.

Checking the spec a bit closer, though:
- min(x, y) is defined as Returns y if y  x, otherwise it returns x
- max(x, y) is defined as Returns y if x  y, otherwise it returns x.

All comparisons with NaN should AFAIK fail, making both the first and
second comparison return NaN, if NaN were to be properly supported.
So my initial analysis was wrong. However, all of the following
permutations of the same logic would still be inconsistent.

min(max(0.0, x), 1.0)
max(min(1.0, x), 0.0)
min(1.0, max(0.0, x))
max(0.0, min(1.0, x))
min(1.0, max(x, 0.0))
max(0.0, min(x, 1.0))

I don't understand the code well enough to figure out if the patch
optimizes these, though.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [v2 PATCH 09/16] glsl: Optimize clamp(x, 0, 1) as saturate(x)

2014-07-08 Thread Abdiel Janulgue 

On 08.07.2014 09:47, Erik Faye-Lund wrote:
 On Tue, Jul 8, 2014 at 8:22 AM, Matt Turner matts...@gmail.com wrote:
 On Mon, Jul 7, 2014 at 11:00 PM, Erik Faye-Lund kusmab...@gmail.com wrote:
 On Mon, Jul 7, 2014 at 7:18 PM, Matt Turner matts...@gmail.com wrote:
 This comment tripped me up for a second. This really means that you've
 found either

   - min(max(x, 0.0), 1.0); or
   - max(min(x, 1.0), 0.0)

 Hmm, but are optimizing both of these to saturate OK? Shouldn't
 min(max(NaN, 0.0), 1.0) give 0.0, whereas max(min(NaN, 1.0), 0.0) give
 1.0?

 Under standard IEEE rules, wouldn't the NaN propagate through all of
 these expressions?

 The GLSL 4.40 spec says

 Operations and built-in functions that operate on a NaN are not required to
  return a NaN as the result.

 So it seems like we have a lot of flexibility here. Is there some text
 I'm missing?
 
 I think the point about flexibility is a bit weak or even
 misunderstood: surely this applies to the built-ins used, not what we
 generate after optimizing. So if we chose to do that, we'd need to
 prevent min and max from propagating NaN even when issued stand-alone,
 which might negatively impact performance in some ISAs.
 
 As to why I reacted, I was just remembering that I read somewhere (one
 of Humus' low-level shader optimization paper, IIRC) that the HLSL
 compiler refused to do some similar optimizations for NaN-reasons.
 
 Checking the spec a bit closer, though:
 - min(x, y) is defined as Returns y if y  x, otherwise it returns x
 - max(x, y) is defined as Returns y if x  y, otherwise it returns x.
 
 All comparisons with NaN should AFAIK fail, making both the first and
 second comparison return NaN, if NaN were to be properly supported.
 So my initial analysis was wrong. However, all of the following
 permutations of the same logic would still be inconsistent.
 
 min(max(0.0, x), 1.0)
 max(min(1.0, x), 0.0)
 min(1.0, max(0.0, x))
 max(0.0, min(1.0, x))
 min(1.0, max(x, 0.0))
 max(0.0, min(x, 1.0))
 
 I don't understand the code well enough to figure out if the patch
 optimizes these, though.

The code actually goes through all 8 commutative variations and tries to
optimize if it spots one of these conditions.

 ___
 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

Re: [Mesa-dev] [v2 PATCH 09/16] glsl: Optimize clamp(x, 0, 1) as saturate(x)

2014-07-08 Thread Roland Scheidegger 
Am 08.07.2014 08:22, schrieb Matt Turner:
 On Mon, Jul 7, 2014 at 11:00 PM, Erik Faye-Lund kusmab...@gmail.com wrote:
 On Mon, Jul 7, 2014 at 7:18 PM, Matt Turner matts...@gmail.com wrote:
 This comment tripped me up for a second. This really means that you've
 found either

   - min(max(x, 0.0), 1.0); or
   - max(min(x, 1.0), 0.0)

 Hmm, but are optimizing both of these to saturate OK? Shouldn't
 min(max(NaN, 0.0), 1.0) give 0.0, whereas max(min(NaN, 1.0), 0.0) give
 1.0?
 
 Under standard IEEE rules, wouldn't the NaN propagate through all of
 these expressions?
not under ieee754-2008 rules, no. min/max return the other number if one
of them is a NaN. This is enforced by d3d10 so at least d3d10 capable
hardware is going to honor that.
http://msdn.microsoft.com/en-us/library/windows/desktop/cc308050%28v=vs.85%29.aspx
The order of min/max though indeed matters.

 
 The GLSL 4.40 spec says
 
 Operations and built-in functions that operate on a NaN are not required to
  return a NaN as the result.
 
 So it seems like we have a lot of flexibility here. Is there some text
 I'm missing?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [v2 PATCH 09/16] glsl: Optimize clamp(x, 0, 1) as saturate(x)

2014-07-08 Thread Erik Faye-Lund 
On Tue, Jul 8, 2014 at 5:44 PM, Roland Scheidegger srol...@vmware.com wrote:
 Am 08.07.2014 08:22, schrieb Matt Turner:
 On Mon, Jul 7, 2014 at 11:00 PM, Erik Faye-Lund kusmab...@gmail.com wrote:
 On Mon, Jul 7, 2014 at 7:18 PM, Matt Turner matts...@gmail.com wrote:
 This comment tripped me up for a second. This really means that you've
 found either

   - min(max(x, 0.0), 1.0); or
   - max(min(x, 1.0), 0.0)

 Hmm, but are optimizing both of these to saturate OK? Shouldn't
 min(max(NaN, 0.0), 1.0) give 0.0, whereas max(min(NaN, 1.0), 0.0) give
 1.0?

 Under standard IEEE rules, wouldn't the NaN propagate through all of
 these expressions?
 not under ieee754-2008 rules, no. min/max return the other number if one
 of them is a NaN.

I'm not entirely sure what IEEE754-2008 has to say about min/max, as
GLSL already defines those operations (and states that IEEE754
compliance isn't required). I was referring to the definition of the
less-than comparison-operator, which GLSL seems to lean on. And that
seems to always evaluate to 'false'.

 This is enforced by d3d10 so at least d3d10 capable
 hardware is going to honor that.
 http://msdn.microsoft.com/en-us/library/windows/desktop/cc308050%28v=vs.85%29.aspx
 The order of min/max though indeed matters.

Right, this is very useful information, thanks. This makes me think
that the perhaps the dependence on the less-than definition could be
considered a spec-bug. A quick test reveals that NVIDIA's Windows
driver implement both min(0.5, NaN) and min(NaN, 0.5) as returning
0.5. So I'd say: raise an issue with Khronos, and implement DX10
rules.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [v2 PATCH 09/16] glsl: Optimize clamp(x, 0, 1) as saturate(x)

2014-07-08 Thread Roland Scheidegger 
Am 08.07.2014 18:03, schrieb Erik Faye-Lund:
 On Tue, Jul 8, 2014 at 5:44 PM, Roland Scheidegger srol...@vmware.com wrote:
 Am 08.07.2014 08:22, schrieb Matt Turner:
 On Mon, Jul 7, 2014 at 11:00 PM, Erik Faye-Lund kusmab...@gmail.com wrote:
 On Mon, Jul 7, 2014 at 7:18 PM, Matt Turner matts...@gmail.com wrote:
 This comment tripped me up for a second. This really means that you've
 found either

   - min(max(x, 0.0), 1.0); or
   - max(min(x, 1.0), 0.0)

 Hmm, but are optimizing both of these to saturate OK? Shouldn't
 min(max(NaN, 0.0), 1.0) give 0.0, whereas max(min(NaN, 1.0), 0.0) give
 1.0?

 Under standard IEEE rules, wouldn't the NaN propagate through all of
 these expressions?
 not under ieee754-2008 rules, no. min/max return the other number if one
 of them is a NaN.
 
 I'm not entirely sure what IEEE754-2008 has to say about min/max, as
 GLSL already defines those operations (and states that IEEE754
 compliance isn't required). I was referring to the definition of the
 less-than comparison-operator, which GLSL seems to lean on. And that
 seems to always evaluate to 'false'.
Oh right I didn't notice min/max are defined that way. I guess that's
due to older specs - pre glsl 4 NaN support wasn't really required and
just about anything involving NaNs or Infs was ok to give undefined
results anyway (and some hw indeed didn't have NaN support).
Even current glsl version is not very strict there, still saying things
like NaNs are not required to be generated and Operations and
built-in functions that operate on a NaN are not required to return a
NaN as the result. I am not entirely sure why the spec wasn't tightened
up a bit, but maybe some glsl 4+ capable hw exists which doesn't
implement d3d10 rules.

 
 This is enforced by d3d10 so at least d3d10 capable
 hardware is going to honor that.
 https://urldefense.proofpoint.com/v1/url?u=http://msdn.microsoft.com/en-us/library/windows/desktop/cc308050%28v%3Dvs.85%29.aspxk=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0Ar=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0Am=9dVgmTYumlj5DwQp2o0tjv4fTQpBhA%2FCDiMURzjOadM%3D%0As=f153cbe35b73f7fdb09fc50f8cfab56651bf346257838bc32ad35a721110fe67
 The order of min/max though indeed matters.
 
 Right, this is very useful information, thanks. This makes me think
 that the perhaps the dependence on the less-than definition could be
 considered a spec-bug. A quick test reveals that NVIDIA's Windows
 driver implement both min(0.5, NaN) and min(NaN, 0.5) as returning
 0.5. So I'd say: raise an issue with Khronos, and implement DX10
 rules
 

I agree that this looks like a bug, at the very least (given the very
lax NaN requirements in general) I think it should allow the d3d10
result if it doesn't do already (it is not obvious to me if the glsl
less-than operator really needs to adhere to ieee754 spec wrt NaNs).

Roland
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [v2 PATCH 09/16] glsl: Optimize clamp(x, 0, 1) as saturate(x)

2014-07-07 Thread Abdiel Janulgue 
v2: Check that the base type is float (Ian Romanick)

Signed-off-by: Abdiel Janulgue abdiel.janul...@linux.intel.com
---
 src/glsl/opt_algebraic.cpp | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/src/glsl/opt_algebraic.cpp b/src/glsl/opt_algebraic.cpp
index ac7514a..2ad561c 100644
--- a/src/glsl/opt_algebraic.cpp
+++ b/src/glsl/opt_algebraic.cpp
@@ -614,6 +614,40 @@ ir_algebraic_visitor::handle_expression(ir_expression *ir)
 
   break;
 
+   case ir_binop_min:
+   case ir_binop_max:
+  if (ir-type-base_type != GLSL_TYPE_FLOAT)
+ break;
+  /* Replace min(max) operations and its commutative combinations with
+   * a saturate operation
+   */
+  for (int op = 0; op  2; op++) {
+ ir_expression *minmax = op_expr[op];
+ ir_constant *outer_const = op_const[(op == 0) ? 1 : 0];
+ ir_expression_operation op_cond = (ir-operation == ir_binop_max) ?
+ir_binop_min : ir_binop_max;
+
+ if (!minmax || !outer_const || (minmax-operation != op_cond))
+continue;
+
+ /* Found a min/max operation. Now try to see if its operands
+  * meet our conditions that we can do just a single saturate operation
+  */
+ for (int minmax_op = 0; minmax_op  2; minmax_op++) {
+ir_rvalue *inner_val_a = minmax-operands[minmax_op];
+ir_rvalue *inner_val_b = minmax-operands[(minmax_op == 0) ? 1 : 
0];
+
+if (!inner_val_a || !inner_val_b)
+   continue;
+
+/* Found a min (max(x, 0), 1.0) */
+if (outer_const-is_one()  inner_val_a-is_zero())
+   return saturate(inner_val_b);
+ }
+  }
+
+  break;
+
case ir_unop_rcp:
   if (op_expr[0]  op_expr[0]-operation == ir_unop_rcp)
 return op_expr[0]-operands[0];
-- 
1.9.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [v2 PATCH 09/16] glsl: Optimize clamp(x, 0, 1) as saturate(x)

2014-07-07 Thread Matt Turner 
On Mon, Jul 7, 2014 at 6:57 AM, Abdiel Janulgue
abdiel.janul...@linux.intel.com wrote:
 v2: Check that the base type is float (Ian Romanick)

 Signed-off-by: Abdiel Janulgue abdiel.janul...@linux.intel.com
 ---
  src/glsl/opt_algebraic.cpp | 34 ++
  1 file changed, 34 insertions(+)

 diff --git a/src/glsl/opt_algebraic.cpp b/src/glsl/opt_algebraic.cpp
 index ac7514a..2ad561c 100644
 --- a/src/glsl/opt_algebraic.cpp
 +++ b/src/glsl/opt_algebraic.cpp
 @@ -614,6 +614,40 @@ ir_algebraic_visitor::handle_expression(ir_expression 
 *ir)

break;

 +   case ir_binop_min:
 +   case ir_binop_max:
 +  if (ir-type-base_type != GLSL_TYPE_FLOAT)
 + break;
 +  /* Replace min(max) operations and its commutative combinations with
 +   * a saturate operation
 +   */
 +  for (int op = 0; op  2; op++) {
 + ir_expression *minmax = op_expr[op];
 + ir_constant *outer_const = op_const[(op == 0) ? 1 : 0];

We use (1 - op) in other places in opt_algebraic. (Same comment below)

 + ir_expression_operation op_cond = (ir-operation == ir_binop_max) ?
 +ir_binop_min : ir_binop_max;
 +
 + if (!minmax || !outer_const || (minmax-operation != op_cond))
 +continue;
 +
 + /* Found a min/max operation. Now try to see if its operands
 +  * meet our conditions that we can do just a single saturate 
 operation
 +  */
 + for (int minmax_op = 0; minmax_op  2; minmax_op++) {
 +ir_rvalue *inner_val_a = minmax-operands[minmax_op];
 +ir_rvalue *inner_val_b = minmax-operands[(minmax_op == 0) ? 1 : 
 0];
 +
 +if (!inner_val_a || !inner_val_b)
 +   continue;
 +
 +/* Found a min (max(x, 0), 1.0) */

This comment tripped me up for a second. This really means that you've
found either

  - min(max(x, 0.0), 1.0); or
  - max(min(x, 1.0), 0.0)

Change the comment to something that says either of these are
acceptable. (Also make the spacing consistent. There's a space after
the min. And Make the floating point literals consistent with .0 on
both)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev