Re: [Mesa-dev] [PATCH 2/2] util: Make CLAMP turn NaN into MIN.

[Marek Olšák]

On Tue, Jul 25, 2017 at 8:12 AM, Roland Scheidegger  wrote:
> Am 24.07.2017 um 11:44 schrieb Michel Dänzer:
>> On 14/07/17 10:01 PM, Marek Olšák wrote:
>>> Reviewed-by: Marek Olšák >
>>
>> This change broke piglit spec@ext_texture_lod_bias@lodbias for me with
>> radeonsi (but not with llvmpipe).
>>
>>
>
> The S_FIXED function needs some fixing (for r600 too). The compiler
> takes full advantage of the fact that float to unsigned conversion is
> undefined for negative floats, apparently.
> The previous CLAMP would have used the state->lod_bias value if that was
> equal to the MIN value (-16) (so no way for the compiler to screw it up)
> whereas now it will use the -16 value.
> (This also means it was probably broken before, for values which were
> smaller than the min, albeit possibly noone cared as that would have
> been outside the advertized lod bias limits.)
> FWIW gcc generates a really terrible code for this (but that's not
> really dependent on the new or old macro...)
> (There's the same macro in mesa/main macros, but with signed numbers.)

https://patchwork.freedesktop.org/patch/168931/

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

Re: [Mesa-dev] [PATCH 2/2] util: Make CLAMP turn NaN into MIN.

[Roland Scheidegger]

Am 24.07.2017 um 11:44 schrieb Michel Dänzer:
> On 14/07/17 10:01 PM, Marek Olšák wrote:
>> Reviewed-by: Marek Olšák >
> 
> This change broke piglit spec@ext_texture_lod_bias@lodbias for me with
> radeonsi (but not with llvmpipe).
> 
> 

The S_FIXED function needs some fixing (for r600 too). The compiler
takes full advantage of the fact that float to unsigned conversion is
undefined for negative floats, apparently.
The previous CLAMP would have used the state->lod_bias value if that was
equal to the MIN value (-16) (so no way for the compiler to screw it up)
whereas now it will use the -16 value.
(This also means it was probably broken before, for values which were
smaller than the min, albeit possibly noone cared as that would have
been outside the advertized lod bias limits.)
FWIW gcc generates a really terrible code for this (but that's not
really dependent on the new or old macro...)
(There's the same macro in mesa/main macros, but with signed numbers.)

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

Re: [Mesa-dev] [PATCH 2/2] util: Make CLAMP turn NaN into MIN.

[Michel Dänzer]

On 14/07/17 10:01 PM, Marek Olšák wrote:
> Reviewed-by: Marek Olšák >

This change broke piglit spec@ext_texture_lod_bias@lodbias for me with
radeonsi (but not with llvmpipe).


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] util: Make CLAMP turn NaN into MIN.

[Marek Olšák]

Reviewed-by: Marek Olšák 

Marek

On Jul 14, 2017 6:38 AM, "Kenneth Graunke"  wrote:

> The previous implementation of CLAMP() allowed NaN to pass through
> unscathed, by failing both comparisons.  NaN isn't exactly a value
> between MIN and MAX, which can break the assumptions of many callers.
>
> This patch changes CLAMP to convert NaN to MIN, arbitrarily.  Callers
> that need NaN to be handled in a specific manner should probably open
> code something, or use a macro specifically designed to do that.
>
> Section 2.3.4.1 of the OpenGL 4.5 spec says:
>
>"Any representable floating-point value is legal as input to a GL
> command that requires floating-point data. The result of providing a
> value that is not a floating-point number to such a command is
> unspecified, but must not lead to GL interruption or termination.
> In IEEE arithmetic, for example, providing a negative zero or a
> denormalized number to a GL command yields predictable results,
> while providing a NaN or an infinity yields unspecified results."
>
> While CLAMP may apply to more than just GL inputs, it seems reasonable
> to follow those rules, and allow MIN as an "unspecified result".
>
> This prevents assertion failures in i965 when running the games
> "XCOM: Enemy Unknown" and "XCOM: Enemy Within", which call
>
>glTexEnv(GL_TEXTURE_FILTER_CONTROL_EXT, GL_TEXTURE_LOD_BIAS_EXT,
> -nan(0x73));
>
> presumably unintentionally.  i965 clamps the LOD bias to be in range,
> and asserts that it's in the proper range when converting to fixed
> point.  NaN is not, so it crashed.  We'd like to at least avoid that.
>
> Cc: Marek Olšák 
> Cc: Roland Scheidegger 
> Cc: Ian Romanick 
> ---
>  src/gallium/auxiliary/util/u_math.h | 3 ++-
>  src/util/macros.h   | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/gallium/auxiliary/util/u_math.h
> b/src/gallium/auxiliary/util/u_math.h
> index 2ab5f03a662..a441b5457b2 100644
> --- a/src/gallium/auxiliary/util/u_math.h
> +++ b/src/gallium/auxiliary/util/u_math.h
> @@ -605,8 +605,9 @@ util_memcpy_cpu_to_le32(void * restrict dest, const
> void * restrict src, size_t
>  /**
>   * Clamp X to [MIN, MAX].
>   * This is a macro to allow float, int, uint, etc. types.
> + * We arbitrarily turn NaN into MIN.
>   */
> -#define CLAMP( X, MIN, MAX )  ( (X)<(MIN) ? (MIN) : ((X)>(MAX) ? (MAX) :
> (X)) )
> +#define CLAMP( X, MIN, MAX )  ( (X)>(MIN) ? ((X)>(MAX) ? (MAX) : (X)) :
> (MIN) )
>
>  #define MIN2( A, B )   ( (A)<(B) ? (A) : (B) )
>  #define MAX2( A, B )   ( (A)>(B) ? (A) : (B) )
> diff --git a/src/util/macros.h b/src/util/macros.h
> index a10f1de8145..a66f1bfed07 100644
> --- a/src/util/macros.h
> +++ b/src/util/macros.h
> @@ -244,8 +244,8 @@ do {   \
>  /** Compute ceiling of integer quotient of A divided by B. */
>  #define DIV_ROUND_UP( A, B )  ( (A) % (B) == 0 ? (A)/(B) : (A)/(B)+1 )
>
> -/** Clamp X to [MIN,MAX] */
> -#define CLAMP( X, MIN, MAX )  ( (X)<(MIN) ? (MIN) : ((X)>(MAX) ? (MAX) :
> (X)) )
> +/** Clamp X to [MIN,MAX].  Turn NaN into MIN, arbitrarily. */
> +#define CLAMP( X, MIN, MAX )  ( (X)>(MIN) ? ((X)>(MAX) ? (MAX) : (X)) :
> (MIN) )
>
>  /** Minimum of two values: */
>  #define MIN2( A, B )   ( (A)<(B) ? (A) : (B) )
> --
> 2.13.3
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] util: Make CLAMP turn NaN into MIN.

[Chris Wilson]

Quoting Roland Scheidegger (2017-07-14 13:22:27)
> Reviewed-by: Roland Scheidegger 
> 
> Interesting side-effect there with the results being different if max >
> min. But hopefully not an issue anywhere else...

Is it worth a gccism to check?

#ifdef __GNUC__
#define CLAMP(x, min, max) ({
typeof(x) __min = (min);
typeof(x) __max = (max);
typeof(x) __x = (x);
assert(__max >= __min);
 __x > __min ? __x > __max ? __max : __x : __min;
})
#else
...
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] util: Make CLAMP turn NaN into MIN.

[Roland Scheidegger]

Reviewed-by: Roland Scheidegger 

Interesting side-effect there with the results being different if max >
min. But hopefully not an issue anywhere else...

Roland

Am 14.07.2017 um 06:38 schrieb Kenneth Graunke:
> The previous implementation of CLAMP() allowed NaN to pass through
> unscathed, by failing both comparisons.  NaN isn't exactly a value
> between MIN and MAX, which can break the assumptions of many callers.
> 
> This patch changes CLAMP to convert NaN to MIN, arbitrarily.  Callers
> that need NaN to be handled in a specific manner should probably open
> code something, or use a macro specifically designed to do that.
> 
> Section 2.3.4.1 of the OpenGL 4.5 spec says:
> 
>"Any representable floating-point value is legal as input to a GL
> command that requires floating-point data. The result of providing a
> value that is not a floating-point number to such a command is
> unspecified, but must not lead to GL interruption or termination.
> In IEEE arithmetic, for example, providing a negative zero or a
> denormalized number to a GL command yields predictable results,
> while providing a NaN or an infinity yields unspecified results."
> 
> While CLAMP may apply to more than just GL inputs, it seems reasonable
> to follow those rules, and allow MIN as an "unspecified result".
> 
> This prevents assertion failures in i965 when running the games
> "XCOM: Enemy Unknown" and "XCOM: Enemy Within", which call
> 
>glTexEnv(GL_TEXTURE_FILTER_CONTROL_EXT, GL_TEXTURE_LOD_BIAS_EXT,
> -nan(0x73));
> 
> presumably unintentionally.  i965 clamps the LOD bias to be in range,
> and asserts that it's in the proper range when converting to fixed
> point.  NaN is not, so it crashed.  We'd like to at least avoid that.
> 
> Cc: Marek Olšák 
> Cc: Roland Scheidegger 
> Cc: Ian Romanick 
> ---
>  src/gallium/auxiliary/util/u_math.h | 3 ++-
>  src/util/macros.h   | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/util/u_math.h 
> b/src/gallium/auxiliary/util/u_math.h
> index 2ab5f03a662..a441b5457b2 100644
> --- a/src/gallium/auxiliary/util/u_math.h
> +++ b/src/gallium/auxiliary/util/u_math.h
> @@ -605,8 +605,9 @@ util_memcpy_cpu_to_le32(void * restrict dest, const void 
> * restrict src, size_t
>  /**
>   * Clamp X to [MIN, MAX].
>   * This is a macro to allow float, int, uint, etc. types.
> + * We arbitrarily turn NaN into MIN.
>   */
> -#define CLAMP( X, MIN, MAX )  ( (X)<(MIN) ? (MIN) : ((X)>(MAX) ? (MAX) : 
> (X)) )
> +#define CLAMP( X, MIN, MAX )  ( (X)>(MIN) ? ((X)>(MAX) ? (MAX) : (X)) : 
> (MIN) )
>  
>  #define MIN2( A, B )   ( (A)<(B) ? (A) : (B) )
>  #define MAX2( A, B )   ( (A)>(B) ? (A) : (B) )
> diff --git a/src/util/macros.h b/src/util/macros.h
> index a10f1de8145..a66f1bfed07 100644
> --- a/src/util/macros.h
> +++ b/src/util/macros.h
> @@ -244,8 +244,8 @@ do {   \
>  /** Compute ceiling of integer quotient of A divided by B. */
>  #define DIV_ROUND_UP( A, B )  ( (A) % (B) == 0 ? (A)/(B) : (A)/(B)+1 )
>  
> -/** Clamp X to [MIN,MAX] */
> -#define CLAMP( X, MIN, MAX )  ( (X)<(MIN) ? (MIN) : ((X)>(MAX) ? (MAX) : 
> (X)) )
> +/** Clamp X to [MIN,MAX].  Turn NaN into MIN, arbitrarily. */
> +#define CLAMP( X, MIN, MAX )  ( (X)>(MIN) ? ((X)>(MAX) ? (MAX) : (X)) : 
> (MIN) )
>  
>  /** Minimum of two values: */
>  #define MIN2( A, B )   ( (A)<(B) ? (A) : (B) )
> 

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

[Mesa-dev] [PATCH 2/2] util: Make CLAMP turn NaN into MIN.

[Kenneth Graunke]

The previous implementation of CLAMP() allowed NaN to pass through
unscathed, by failing both comparisons.  NaN isn't exactly a value
between MIN and MAX, which can break the assumptions of many callers.

This patch changes CLAMP to convert NaN to MIN, arbitrarily.  Callers
that need NaN to be handled in a specific manner should probably open
code something, or use a macro specifically designed to do that.

Section 2.3.4.1 of the OpenGL 4.5 spec says:

   "Any representable floating-point value is legal as input to a GL
command that requires floating-point data. The result of providing a
value that is not a floating-point number to such a command is
unspecified, but must not lead to GL interruption or termination.
In IEEE arithmetic, for example, providing a negative zero or a
denormalized number to a GL command yields predictable results,
while providing a NaN or an infinity yields unspecified results."

While CLAMP may apply to more than just GL inputs, it seems reasonable
to follow those rules, and allow MIN as an "unspecified result".

This prevents assertion failures in i965 when running the games
"XCOM: Enemy Unknown" and "XCOM: Enemy Within", which call

   glTexEnv(GL_TEXTURE_FILTER_CONTROL_EXT, GL_TEXTURE_LOD_BIAS_EXT,
-nan(0x73));

presumably unintentionally.  i965 clamps the LOD bias to be in range,
and asserts that it's in the proper range when converting to fixed
point.  NaN is not, so it crashed.  We'd like to at least avoid that.

Cc: Marek Olšák 
Cc: Roland Scheidegger 
Cc: Ian Romanick 
---
 src/gallium/auxiliary/util/u_math.h | 3 ++-
 src/util/macros.h   | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/gallium/auxiliary/util/u_math.h 
b/src/gallium/auxiliary/util/u_math.h
index 2ab5f03a662..a441b5457b2 100644
--- a/src/gallium/auxiliary/util/u_math.h
+++ b/src/gallium/auxiliary/util/u_math.h
@@ -605,8 +605,9 @@ util_memcpy_cpu_to_le32(void * restrict dest, const void * 
restrict src, size_t
 /**
  * Clamp X to [MIN, MAX].
  * This is a macro to allow float, int, uint, etc. types.
+ * We arbitrarily turn NaN into MIN.
  */
-#define CLAMP( X, MIN, MAX )  ( (X)<(MIN) ? (MIN) : ((X)>(MAX) ? (MAX) : (X)) )
+#define CLAMP( X, MIN, MAX )  ( (X)>(MIN) ? ((X)>(MAX) ? (MAX) : (X)) : (MIN) )
 
 #define MIN2( A, B )   ( (A)<(B) ? (A) : (B) )
 #define MAX2( A, B )   ( (A)>(B) ? (A) : (B) )
diff --git a/src/util/macros.h b/src/util/macros.h
index a10f1de8145..a66f1bfed07 100644
--- a/src/util/macros.h
+++ b/src/util/macros.h
@@ -244,8 +244,8 @@ do {   \
 /** Compute ceiling of integer quotient of A divided by B. */
 #define DIV_ROUND_UP( A, B )  ( (A) % (B) == 0 ? (A)/(B) : (A)/(B)+1 )
 
-/** Clamp X to [MIN,MAX] */
-#define CLAMP( X, MIN, MAX )  ( (X)<(MIN) ? (MIN) : ((X)>(MAX) ? (MAX) : (X)) )
+/** Clamp X to [MIN,MAX].  Turn NaN into MIN, arbitrarily. */
+#define CLAMP( X, MIN, MAX )  ( (X)>(MIN) ? ((X)>(MAX) ? (MAX) : (X)) : (MIN) )
 
 /** Minimum of two values: */
 #define MIN2( A, B )   ( (A)<(B) ? (A) : (B) )
-- 
2.13.3

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