Re: [Mesa-dev] [PATCH v2] Fixing an x86 FPU bug.
-Original Message-
From: Neil Roberts [mailto:n...@linux.intel.com]
Sent: Wednesday, February 04, 2015 4:02 PM
To: Predut, Marius; mesa-dev@lists.freedesktop.org
Cc: mesa-sta...@lists.freedesktop.org
Subject: Re: [Mesa-dev] [PATCH v2] Fixing an x86 FPU bug.
Hi Marius,
This patch does indeed make the Piglit test pass but it doesn't seem like a
complete fix. It looks like there are still a number of places that are
copying via floats that the test wouldn't catch. There are also lots of places
that are still using GLfloat to store the attribute values and then these are
cast to gl_constant_value* whenever they are used in combination with the
places that the patch does change. I think if we wanted to do a complete patch
then we should really change all places that are storing attribute values to
use gl_constant_value to make it clear that they aren't really just floats.
If it is too much of a change just to fix this relatively minor bug then
perhaps it would be better to do a simpler change that just fixes this one
case using a memcpy or something instead of changing so much code. I don't
really like the idea of only changing some of the places to gl_constant_value
and leaving the rest as GLfloat because it seems even more confusing that way.
I don't think it makes sense to add this patch to the stable branch. The bug
has presumably been there since the introduction of integer attributes in 2012
(see acf438f537) and nobody has complained so it doesn't seem particularly
urgent. The patch is non-trivial so I think there is a risk it will introduce
more bugs.
Yes, it is too much of changes to use gl constant value everywhere. But I will
came with more versions.
For review may be it will be better and one of the solutions will be accepted.
I thinking to prepare 2 versions, one with minimal casting and
one that will include full changes with gl_constant_value and maybe one that
includes a different elaboration/solution.(may be using memcpy ?)
Also I see many comments related to expressions like *.f.
I guess using union is enough to avoid x86 FPU registers even if we have
expression like *.f
In this case ,I suppose the compiler avoid optimization and it NOT use those
registers.
I've made some comments inline below.
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).
The defect was checked with and without -O3 compiler flag.
CC: mesa-sta...@lists.freedesktop.org
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| 47 --
---
src/mesa/vbo/vbo_attrib_tmp.h | 20 ++
src/mesa/vbo/vbo_exec.h |3 ++-
src/mesa/vbo/vbo_exec_api.c | 31 +--
src/mesa/vbo/vbo_exec_eval.c | 22 ++-
src/mesa/vbo/vbo_save_api.c | 16 +++---
src/mesa/vbo/vbo_save_draw.c |4 ++--
8 files changed, 90 insertions(+), 56 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..2651ffc 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
Re: [Mesa-dev] [PATCH v2] Fixing an x86 FPU bug.
Hi Marius,
This patch does indeed make the Piglit test pass but it doesn't seem
like a complete fix. It looks like there are still a number of places
that are copying via floats that the test wouldn't catch. There are also
lots of places that are still using GLfloat to store the attribute
values and then these are cast to gl_constant_value* whenever they are
used in combination with the places that the patch does change. I think
if we wanted to do a complete patch then we should really change all
places that are storing attribute values to use gl_constant_value to
make it clear that they aren't really just floats.
If it is too much of a change just to fix this relatively minor bug then
perhaps it would be better to do a simpler change that just fixes this
one case using a memcpy or something instead of changing so much code. I
don't really like the idea of only changing some of the places to
gl_constant_value and leaving the rest as GLfloat because it seems even
more confusing that way.
I don't think it makes sense to add this patch to the stable branch. The
bug has presumably been there since the introduction of integer
attributes in 2012 (see acf438f537) and nobody has complained so it
doesn't seem particularly urgent. The patch is non-trivial so I think
there is a risk it will introduce more bugs.
I've made some comments inline below.
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).
The defect was checked with and without -O3 compiler flag.
CC: mesa-sta...@lists.freedesktop.org
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| 47
-
src/mesa/vbo/vbo_attrib_tmp.h | 20 ++
src/mesa/vbo/vbo_exec.h |3 ++-
src/mesa/vbo/vbo_exec_api.c | 31 +--
src/mesa/vbo/vbo_exec_eval.c | 22 ++-
src/mesa/vbo/vbo_save_api.c | 16 +++---
src/mesa/vbo/vbo_save_draw.c |4 ++--
8 files changed, 90 insertions(+), 56 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..2651ffc 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.
*
@@ -382,6 +382,15 @@ do {\
V[3] = V3; \
} while(0)
+/** Assignment union*/
+#define ASSIGN_4V_UNION( V, V0, V1, V2, V3 ) \
+do {\
+V[0].f = V0; \
+V[1].f = V1; \
+V[2].f = V2; \
+V[3].f = V3; \
+} while(0)
+
I think it would be better not to have this macro and just use the
ASSIGN_4V macro to copy the gl_constant_value structs directly. This
macro is copying the union values via a floating-point operation which
is what we're trying to avoid. Technically it doesn't really matter in
this case because
[Mesa-dev] [PATCH v2] Fixing an x86 FPU bug.
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).
The defect was checked with and without -O3 compiler flag.
CC: mesa-sta...@lists.freedesktop.org
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| 47 -
src/mesa/vbo/vbo_attrib_tmp.h | 20 ++
src/mesa/vbo/vbo_exec.h |3 ++-
src/mesa/vbo/vbo_exec_api.c | 31 +--
src/mesa/vbo/vbo_exec_eval.c | 22 ++-
src/mesa/vbo/vbo_save_api.c | 16 +++---
src/mesa/vbo/vbo_save_draw.c |4 ++--
8 files changed, 90 insertions(+), 56 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..2651ffc 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.
*
@@ -382,6 +382,15 @@ do {\
V[3] = V3; \
} while(0)
+/** Assignment union*/
+#define ASSIGN_4V_UNION( V, V0, V1, V2, V3 ) \
+do {\
+V[0].f = V0; \
+V[1].f = V1; \
+V[2].f = V2; \
+V[3].f = V3; \
+} while(0)
+
/*@}*/
@@ -620,23 +629,23 @@ do { \
* The default values are chosen based on \p type.
*/
static inline void
-COPY_CLEAN_4V_TYPE_AS_FLOAT(GLfloat dst[4], int sz, const GLfloat src[4],
+COPY_CLEAN_4V_TYPE_AS_FLOAT(gl_constant_value dst[4], int sz, const
gl_constant_value src[4],
GLenum type)
{
switch (type) {
case GL_FLOAT:
- ASSIGN_4V(dst, 0, 0, 0, 1);
+ ASSIGN_4V_UNION(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_UNION(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_UNION(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 */
+ ASSIGN_4V_UNION(dst, 0.0f, 0.0f, 0.0f, 1.0f); /* silence warnings */
ASSERT(!Unexpected type in COPY_CLEAN_4V_TYPE_AS_FLOAT macro);
}
COPY_SZ_4V(dst, sz, src);
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,
