Re: [Mesa-dev] [PATCH v1] Remove UINT_AS_FLT, INT_AS_FLT, FLOAT_AS_FLT macros.No functional changes, only bug fixed.

2015-01-22 Thread Neil Roberts 
Hi,

The COPY_CLEAN_4V_TYPE_AS_FLOAT still doesn't look right because as the
last step it calls COPY_SZ_4V which will copy its float arguments using
floating-point registers. It seems the piglit test case is still failing
and if I step through with GDB I can see that it is hitting this code
and using the floating-point registers to do the copy. Are you compiling
your Mesa with -O3? It would be worth initially trying to replicate the
test failure and then verifying that the patch fixes at least the piglit
test before posting it.

I agree that this function should be changed to take pointers to
gl_constant_values instead of GLfloats as suggested by Ian in his review
for the previous version.

Regards,
- Neil

marius.pre...@intel.com writes:

 From: Marius Predut marius.pre...@intel.com

 On 32-bit, for floating point operations is used x86 FPU registers instead 
 SSE,
 reason for  when reinterprets an integer as a float result is unexpected
 (modify floats when they are written to memory).

 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668

 Signed-off-by: Marius Predut marius.pre...@intel.com
 ---
  src/mesa/main/context.c   |3 ++-
  src/mesa/main/macros.h|   32 
  src/mesa/vbo/vbo_attrib_tmp.h |   20 
  src/mesa/vbo/vbo_exec.h   |3 ++-
  src/mesa/vbo/vbo_exec_api.c   |   25 -
  src/mesa/vbo/vbo_exec_eval.c  |   22 +-
  src/mesa/vbo/vbo_save_api.c   |   10 +-
  7 files changed, 70 insertions(+), 45 deletions(-)

 diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
 index 400c158..11ab8a9 100644
 --- a/src/mesa/main/context.c
 +++ b/src/mesa/main/context.c
 @@ -134,6 +134,7 @@
  #include math/m_matrix.h
  #include main/dispatch.h /* for _gloffset_COUNT */
  #include uniforms.h
 +#include macros.h
  
  #ifdef USE_SPARC_ASM
  #include sparc/sparc.h
 @@ -656,7 +657,7 @@ _mesa_init_constants(struct gl_constants *consts, gl_api 
 api)
 consts-MaxSamples = 0;
  
 /* GLSL default if NativeIntegers == FALSE */
 -   consts-UniformBooleanTrue = FLT_AS_UINT(1.0f);
 +   consts-UniformBooleanTrue = FLOAT_AS_UNION(1.0f).u;
  
 /* GL_ARB_sync */
 consts-MaxServerWaitTimeout = 0x1fff7fffULL;
 diff --git a/src/mesa/main/macros.h b/src/mesa/main/macros.h
 index cd5f2d6..12c9997 100644
 --- a/src/mesa/main/macros.h
 +++ b/src/mesa/main/macros.h
 @@ -32,6 +32,7 @@
  #define MACROS_H
  
  #include imports.h
 +#include program/prog_parameter.h
  
  
  /**
 @@ -170,27 +171,26 @@ extern GLfloat _mesa_ubyte_to_float_color_tab[256];
   ub = ((GLubyte) F_TO_I((f) * 255.0F))
  #endif
  
 -static inline GLfloat INT_AS_FLT(GLint i)
 +static union gl_constant_value UINT_AS_UNION(GLuint u)
  {
 -   fi_type tmp;
 -   tmp.i = i;
 -   return tmp.f;
 +   union gl_constant_value tmp;
 +   tmp.u = u;
 +   return tmp;
  }
  
 -static inline GLfloat UINT_AS_FLT(GLuint u)
 +static inline union gl_constant_value INT_AS_UNION(GLint i)
  {
 -   fi_type tmp;
 -   tmp.u = u;
 -   return tmp.f;
 +   union gl_constant_value tmp;
 +   tmp.i = i;
 +   return tmp;
  }
  
 -static inline unsigned FLT_AS_UINT(float f)
 +static inline union gl_constant_value FLOAT_AS_UNION(GLfloat f)
  {
 -   fi_type tmp;
 +   union gl_constant_value tmp;
 tmp.f = f;
 -   return tmp.u;
 +   return tmp;
  }
 -
  /**
   * Convert a floating point value to an unsigned fixed point value.
   *
 @@ -628,12 +628,12 @@ COPY_CLEAN_4V_TYPE_AS_FLOAT(GLfloat dst[4], int sz, 
 const GLfloat src[4],
ASSIGN_4V(dst, 0, 0, 0, 1);
break;
 case GL_INT:
 -  ASSIGN_4V(dst, INT_AS_FLT(0), INT_AS_FLT(0),
 - INT_AS_FLT(0), INT_AS_FLT(1));
 +  ASSIGN_4V(dst, INT_AS_UNION(0).f, INT_AS_UNION(0).f,
 +INT_AS_UNION(0).f, INT_AS_UNION(1).f);
break;
 case GL_UNSIGNED_INT:
 -  ASSIGN_4V(dst, UINT_AS_FLT(0), UINT_AS_FLT(0),
 - UINT_AS_FLT(0), UINT_AS_FLT(1));
 +  ASSIGN_4V(dst, UINT_AS_UNION(0).f, UINT_AS_UNION(0).f,
 +UINT_AS_UNION(0).f, UINT_AS_UNION(1).f);
break;
 default:
ASSIGN_4V(dst, 0.0f, 0.0f, 0.0f, 1.0f); /* silence warnings */
 diff --git a/src/mesa/vbo/vbo_attrib_tmp.h b/src/mesa/vbo/vbo_attrib_tmp.h
 index ec66934..17a0a10 100644
 --- a/src/mesa/vbo/vbo_attrib_tmp.h
 +++ b/src/mesa/vbo/vbo_attrib_tmp.h
 @@ -28,6 +28,18 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
  #include util/u_format_r11g11b10f.h
  #include main/varray.h
  
 +
 +/* ATTR */
 +#define ATTR( A, N, T, V0, V1, V2, V3 )ATTR_##T((A), (N), (T), (V0), 
 (V1), (V2), (V3))
 +
 +#define ATTR_GL_UNSIGNED_INT( A, N, T, V0, V1, V2, V3 ) \
 +ATTR_UNION(A, N, T, UINT_AS_UNION(V0), UINT_AS_UNION(V1), 
 UINT_AS_UNION(V2), UINT_AS_UNION(V3))
 +#define ATTR_GL_INT( A, N, T, V0, V1, V2, V3 ) \
 +ATTR_UNION(A, N, T, INT_AS_UNION(V0), INT_AS_UNION(V1), 
 INT_AS_UNION(V2), INT_AS_UNION(V3))
 +#define ATTR_GL_FLOAT( A, N, T,

Re: [Mesa-dev] [PATCH v1] Remove UINT_AS_FLT, INT_AS_FLT, FLOAT_AS_FLT macros.No functional changes, only bug fixed.

2015-01-22 Thread Predut, Marius 
 -Original Message-
 From: Neil Roberts [mailto:n...@linux.intel.com]
 Sent: Thursday, January 22, 2015 3:32 PM
 To: Predut, Marius; mesa-dev@lists.freedesktop.org
 Cc: mesa-sta...@lists.freedesktop.org
 Subject: Re: [Mesa-dev] [PATCH v1] Remove UINT_AS_FLT, INT_AS_FLT,
 FLOAT_AS_FLT macros.No functional changes, only bug fixed.
 
 Hi,
 
 The COPY_CLEAN_4V_TYPE_AS_FLOAT still doesn't look right because as the last
 step it calls COPY_SZ_4V which will copy its float arguments using floating-
 point registers. It seems the piglit test case is still failing and if I step
 through with GDB I can see that it is hitting this code and using the
 floating-point registers to do the copy. Are you compiling your Mesa with -O3?
 It would be worth initially trying to replicate the test failure and then
 verifying that the patch fixes at least the piglit test before posting it.
 
 I agree that this function should be changed to take pointers to
 gl_constant_values instead of GLfloats as suggested by Ian in his review for
 the previous version.
 
 Regards,
 - Neil
 


Indeed, seems the problem is partially fixed. With O3 flag fail.



 marius.pre...@intel.com writes:
 
  From: Marius Predut marius.pre...@intel.com
 
  On 32-bit, for floating point operations is used x86 FPU registers
  instead SSE, reason for  when reinterprets an integer as a float
  result is unexpected (modify floats when they are written to memory).
 
  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668
 
  Signed-off-by: Marius Predut marius.pre...@intel.com
  ---
   src/mesa/main/context.c   |3 ++-
   src/mesa/main/macros.h|   32 
   src/mesa/vbo/vbo_attrib_tmp.h |   20 
   src/mesa/vbo/vbo_exec.h   |3 ++-
   src/mesa/vbo/vbo_exec_api.c   |   25 -
   src/mesa/vbo/vbo_exec_eval.c  |   22 +-
   src/mesa/vbo/vbo_save_api.c   |   10 +-
   7 files changed, 70 insertions(+), 45 deletions(-)
 
  diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c index
  400c158..11ab8a9 100644
  --- a/src/mesa/main/context.c
  +++ b/src/mesa/main/context.c
  @@ -134,6 +134,7 @@
   #include math/m_matrix.h
   #include main/dispatch.h /* for _gloffset_COUNT */  #include
  uniforms.h
  +#include macros.h
 
   #ifdef USE_SPARC_ASM
   #include sparc/sparc.h
  @@ -656,7 +657,7 @@ _mesa_init_constants(struct gl_constants *consts, gl_api
 api)
  consts-MaxSamples = 0;
 
  /* GLSL default if NativeIntegers == FALSE */
  -   consts-UniformBooleanTrue = FLT_AS_UINT(1.0f);
  +   consts-UniformBooleanTrue = FLOAT_AS_UNION(1.0f).u;
 
  /* GL_ARB_sync */
  consts-MaxServerWaitTimeout = 0x1fff7fffULL; diff --git
  a/src/mesa/main/macros.h b/src/mesa/main/macros.h index
  cd5f2d6..12c9997 100644
  --- a/src/mesa/main/macros.h
  +++ b/src/mesa/main/macros.h
  @@ -32,6 +32,7 @@
   #define MACROS_H
 
   #include imports.h
  +#include program/prog_parameter.h
 
 
   /**
  @@ -170,27 +171,26 @@ extern GLfloat _mesa_ubyte_to_float_color_tab[256];
  ub = ((GLubyte) F_TO_I((f) * 255.0F))  #endif
 
  -static inline GLfloat INT_AS_FLT(GLint i)
  +static union gl_constant_value UINT_AS_UNION(GLuint u)
   {
  -   fi_type tmp;
  -   tmp.i = i;
  -   return tmp.f;
  +   union gl_constant_value tmp;
  +   tmp.u = u;
  +   return tmp;
   }
 
  -static inline GLfloat UINT_AS_FLT(GLuint u)
  +static inline union gl_constant_value INT_AS_UNION(GLint i)
   {
  -   fi_type tmp;
  -   tmp.u = u;
  -   return tmp.f;
  +   union gl_constant_value tmp;
  +   tmp.i = i;
  +   return tmp;
   }
 
  -static inline unsigned FLT_AS_UINT(float f)
  +static inline union gl_constant_value FLOAT_AS_UNION(GLfloat f)
   {
  -   fi_type tmp;
  +   union gl_constant_value tmp;
  tmp.f = f;
  -   return tmp.u;
  +   return tmp;
   }
  -
   /**
* Convert a floating point value to an unsigned fixed point value.
*
  @@ -628,12 +628,12 @@ COPY_CLEAN_4V_TYPE_AS_FLOAT(GLfloat dst[4], int sz,
 const GLfloat src[4],
 ASSIGN_4V(dst, 0, 0, 0, 1);
 break;
  case GL_INT:
  -  ASSIGN_4V(dst, INT_AS_FLT(0), INT_AS_FLT(0),
  - INT_AS_FLT(0), INT_AS_FLT(1));
  +  ASSIGN_4V(dst, INT_AS_UNION(0).f, INT_AS_UNION(0).f,
  +INT_AS_UNION(0).f, INT_AS_UNION(1).f);
 break;
  case GL_UNSIGNED_INT:
  -  ASSIGN_4V(dst, UINT_AS_FLT(0), UINT_AS_FLT(0),
  - UINT_AS_FLT(0), UINT_AS_FLT(1));
  +  ASSIGN_4V(dst, UINT_AS_UNION(0).f, UINT_AS_UNION(0).f,
  +UINT_AS_UNION(0).f, UINT_AS_UNION(1).f);
 break;
  default:
 ASSIGN_4V(dst, 0.0f, 0.0f, 0.0f, 1.0f); /* silence warnings */
  diff --git a/src/mesa/vbo/vbo_attrib_tmp.h
  b/src/mesa/vbo/vbo_attrib_tmp.h index ec66934..17a0a10 100644
  --- a/src/mesa/vbo/vbo_attrib_tmp.h
  +++ b/src/mesa/vbo/vbo_attrib_tmp.h
  @@ -28,6 +28,18 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
   #include util

Re: [Mesa-dev] [PATCH v1] Remove UINT_AS_FLT, INT_AS_FLT, FLOAT_AS_FLT macros.No functional changes, only bug fixed.

2015-01-22 Thread Predut, Marius 


 -Original Message-
 From: Kenneth Graunke [mailto:kenn...@whitecape.org]
 Sent: Thursday, January 22, 2015 8:02 AM
 To: mesa-dev@lists.freedesktop.org
 Cc: Predut, Marius; mesa-sta...@lists.freedesktop.org
 Subject: Re: [Mesa-dev] [PATCH v1] Remove UINT_AS_FLT, INT_AS_FLT,
 FLOAT_AS_FLT macros.No functional changes, only bug fixed.
 
 On Thursday, January 22, 2015 12:12:18 AM marius.pre...@intel.com wrote:
  From: Marius Predut marius.pre...@intel.com
 
  On 32-bit, for floating point operations is used x86 FPU registers
  instead SSE, reason for  when reinterprets an integer as a float
  result is unexpected (modify floats when they are written to memory).
 
 
  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668
 
  Signed-off-by: Marius Predut marius.pre...@intel.com
  ---
   src/mesa/main/context.c   |3 ++-
   src/mesa/main/macros.h|   32 
   src/mesa/vbo/vbo_attrib_tmp.h |   20 
   src/mesa/vbo/vbo_exec.h   |3 ++-
   src/mesa/vbo/vbo_exec_api.c   |   25 -
   src/mesa/vbo/vbo_exec_eval.c  |   22 +-
   src/mesa/vbo/vbo_save_api.c   |   10 +-
   7 files changed, 70 insertions(+), 45 deletions(-)
 
 Your commit title says No functional changes, only bug fixed.  This is
 contradictory - a bug fix is a change in behavior, or a functional change.
 
 No functional changes is a phrase used for patches which alter whitespace,
 comments, change style, or move code around while not actually changing the
 result of the code.
 
 Removing the macros is also not really the point of the patch - it's fixing an
 x86 FPU bug.  I would make the commit title something to that effect.
Sure, can you do that. thanks

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

Re: [Mesa-dev] [PATCH v1] Remove UINT_AS_FLT, INT_AS_FLT, FLOAT_AS_FLT macros.No functional changes, only bug fixed.

2015-01-21 Thread Kenneth Graunke 
On Thursday, January 22, 2015 12:12:18 AM marius.pre...@intel.com wrote:
 From: Marius Predut marius.pre...@intel.com
 
 On 32-bit, for floating point operations is used x86 FPU registers instead 
 SSE,
 reason for  when reinterprets an integer as a float result is unexpected
 (modify floats when they are written to memory).

 
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668
 
 Signed-off-by: Marius Predut marius.pre...@intel.com
 ---
  src/mesa/main/context.c   |3 ++-
  src/mesa/main/macros.h|   32 
  src/mesa/vbo/vbo_attrib_tmp.h |   20 
  src/mesa/vbo/vbo_exec.h   |3 ++-
  src/mesa/vbo/vbo_exec_api.c   |   25 -
  src/mesa/vbo/vbo_exec_eval.c  |   22 +-
  src/mesa/vbo/vbo_save_api.c   |   10 +-
  7 files changed, 70 insertions(+), 45 deletions(-)

Your commit title says No functional changes, only bug fixed.  This is
contradictory - a bug fix is a change in behavior, or a functional change.

No functional changes is a phrase used for patches which alter whitespace,
comments, change style, or move code around while not actually changing the
result of the code.

Removing the macros is also not really the point of the patch - it's fixing
an x86 FPU bug.  I would make the commit title something to that effect.

signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v1] Remove UINT_AS_FLT, INT_AS_FLT, FLOAT_AS_FLT macros.No functional changes, only bug fixed.

2015-01-21 Thread marius . predut 
From: Marius Predut marius.pre...@intel.com

On 32-bit, for floating point operations is used x86 FPU registers instead SSE,
reason for  when reinterprets an integer as a float result is unexpected
(modify floats when they are written to memory).

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668

Signed-off-by: Marius Predut marius.pre...@intel.com
---
 src/mesa/main/context.c   |3 ++-
 src/mesa/main/macros.h|   32 
 src/mesa/vbo/vbo_attrib_tmp.h |   20 
 src/mesa/vbo/vbo_exec.h   |3 ++-
 src/mesa/vbo/vbo_exec_api.c   |   25 -
 src/mesa/vbo/vbo_exec_eval.c  |   22 +-
 src/mesa/vbo/vbo_save_api.c   |   10 +-
 7 files changed, 70 insertions(+), 45 deletions(-)

diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
index 400c158..11ab8a9 100644
--- a/src/mesa/main/context.c
+++ b/src/mesa/main/context.c
@@ -134,6 +134,7 @@
 #include math/m_matrix.h
 #include main/dispatch.h /* for _gloffset_COUNT */
 #include uniforms.h
+#include macros.h
 
 #ifdef USE_SPARC_ASM
 #include sparc/sparc.h
@@ -656,7 +657,7 @@ _mesa_init_constants(struct gl_constants *consts, gl_api 
api)
consts-MaxSamples = 0;
 
/* GLSL default if NativeIntegers == FALSE */
-   consts-UniformBooleanTrue = FLT_AS_UINT(1.0f);
+   consts-UniformBooleanTrue = FLOAT_AS_UNION(1.0f).u;
 
/* GL_ARB_sync */
consts-MaxServerWaitTimeout = 0x1fff7fffULL;
diff --git a/src/mesa/main/macros.h b/src/mesa/main/macros.h
index cd5f2d6..12c9997 100644
--- a/src/mesa/main/macros.h
+++ b/src/mesa/main/macros.h
@@ -32,6 +32,7 @@
 #define MACROS_H
 
 #include imports.h
+#include program/prog_parameter.h
 
 
 /**
@@ -170,27 +171,26 @@ extern GLfloat _mesa_ubyte_to_float_color_tab[256];
ub = ((GLubyte) F_TO_I((f) * 255.0F))
 #endif
 
-static inline GLfloat INT_AS_FLT(GLint i)
+static union gl_constant_value UINT_AS_UNION(GLuint u)
 {
-   fi_type tmp;
-   tmp.i = i;
-   return tmp.f;
+   union gl_constant_value tmp;
+   tmp.u = u;
+   return tmp;
 }
 
-static inline GLfloat UINT_AS_FLT(GLuint u)
+static inline union gl_constant_value INT_AS_UNION(GLint i)
 {
-   fi_type tmp;
-   tmp.u = u;
-   return tmp.f;
+   union gl_constant_value tmp;
+   tmp.i = i;
+   return tmp;
 }
 
-static inline unsigned FLT_AS_UINT(float f)
+static inline union gl_constant_value FLOAT_AS_UNION(GLfloat f)
 {
-   fi_type tmp;
+   union gl_constant_value tmp;
tmp.f = f;
-   return tmp.u;
+   return tmp;
 }
-
 /**
  * Convert a floating point value to an unsigned fixed point value.
  *
@@ -628,12 +628,12 @@ COPY_CLEAN_4V_TYPE_AS_FLOAT(GLfloat dst[4], int sz, const 
GLfloat src[4],
   ASSIGN_4V(dst, 0, 0, 0, 1);
   break;
case GL_INT:
-  ASSIGN_4V(dst, INT_AS_FLT(0), INT_AS_FLT(0),
- INT_AS_FLT(0), INT_AS_FLT(1));
+  ASSIGN_4V(dst, INT_AS_UNION(0).f, INT_AS_UNION(0).f,
+INT_AS_UNION(0).f, INT_AS_UNION(1).f);
   break;
case GL_UNSIGNED_INT:
-  ASSIGN_4V(dst, UINT_AS_FLT(0), UINT_AS_FLT(0),
- UINT_AS_FLT(0), UINT_AS_FLT(1));
+  ASSIGN_4V(dst, UINT_AS_UNION(0).f, UINT_AS_UNION(0).f,
+UINT_AS_UNION(0).f, UINT_AS_UNION(1).f);
   break;
default:
   ASSIGN_4V(dst, 0.0f, 0.0f, 0.0f, 1.0f); /* silence warnings */
diff --git a/src/mesa/vbo/vbo_attrib_tmp.h b/src/mesa/vbo/vbo_attrib_tmp.h
index ec66934..17a0a10 100644
--- a/src/mesa/vbo/vbo_attrib_tmp.h
+++ b/src/mesa/vbo/vbo_attrib_tmp.h
@@ -28,6 +28,18 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
 #include util/u_format_r11g11b10f.h
 #include main/varray.h
 
+
+/* ATTR */
+#define ATTR( A, N, T, V0, V1, V2, V3 )  ATTR_##T((A), (N), (T), (V0), 
(V1), (V2), (V3))
+
+#define ATTR_GL_UNSIGNED_INT( A, N, T, V0, V1, V2, V3 ) \
+ATTR_UNION(A, N, T, UINT_AS_UNION(V0), UINT_AS_UNION(V1), 
UINT_AS_UNION(V2), UINT_AS_UNION(V3))
+#define ATTR_GL_INT( A, N, T, V0, V1, V2, V3 ) \
+ATTR_UNION(A, N, T, INT_AS_UNION(V0), INT_AS_UNION(V1), INT_AS_UNION(V2), 
INT_AS_UNION(V3))
+#define ATTR_GL_FLOAT( A, N, T, V0, V1, V2, V3 )   \
+ATTR_UNION(A, N, T, FLOAT_AS_UNION(V0), FLOAT_AS_UNION(V1), 
FLOAT_AS_UNION(V2), FLOAT_AS_UNION(V3))
+
+
 /* float */
 #define ATTR1FV( A, V ) ATTR( A, 1, GL_FLOAT, (V)[0], 0, 0, 1 )
 #define ATTR2FV( A, V ) ATTR( A, 2, GL_FLOAT, (V)[0], (V)[1], 0, 1 )
@@ -41,8 +53,8 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 /* int */
 #define ATTRI( A, N, X, Y, Z, W) ATTR( A, N, GL_INT, \
-   INT_AS_FLT(X), INT_AS_FLT(Y), \
-   INT_AS_FLT(Z), INT_AS_FLT(W) )
+   X, Y, \
+   Z, W )
 
 #define ATTR2IV( A, V ) ATTRI( A, 2, (V)[0], (V)[1], 0, 1 )
 #define ATTR3IV( A, V ) ATTRI( A, 3, (V)[0], (V)[1], (V)[2], 1 )
@@ -56,8 +68,8 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 /* uint */
 #define ATTRUI( A, N, X, Y,