[Mesa-dev] [mesa-dev][PATCH] Remove UINT_AS_FLT, INT_AS_FLT, FLOAT_AS_FLT macros.No functional changes, only bug fixed.
Predut, Marius
marius.predut at intel.com
Thu Jan 22 02:10:30 PST 2015
> -----Original Message-----
> From: Ian Romanick [mailto:idr at freedesktop.org]
> Sent: Wednesday, January 21, 2015 5:22 AM
> To: Predut, Marius; mesa-dev at lists.freedesktop.org
> Cc: mesa-stable at lists.freedesktop.org
> Subject: Re: [Mesa-dev] [mesa-dev][PATCH] Remove UINT_AS_FLT, INT_AS_FLT,
> FLOAT_AS_FLT macros.No functional changes, only bug fixed.
>
> On 01/20/2015 07:30 AM, marius.predut at intel.com wrote:
> > From: Marius Predut <marius.predut at 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).
> > This fixes https://bugs.freedesktop.org/show_bug.cgi?id=82668
> >
> > Reviewed-by: Roberts, Neil S<neil.s.roberts at intel.com>
>
> This should be formatted as:
>
> Reviewed-by: Neil Roberts <neil.s.roberts at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668
>
> > ---
> > src/mesa/main/context.c | 2 +-
> > src/mesa/main/macros.h | 29 ++-------------------------
> > src/mesa/vbo/vbo_attrib_tmp.h | 43 +++++++++++++++++++++++++++++++++++++-
> ---
> > 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, 78 insertions(+), 56 deletions(-)
> >
> > diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c index
> > 400c158..3007491 100644
> > --- a/src/mesa/main/context.c
> > +++ b/src/mesa/main/context.c
> > @@ -656,7 +656,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 = 1;
>
> As Jason mentioned, this hunk must be dropped.
Jason comments:
Here can be use 0x3f800000 (the binary representation of 1.0f) Or, could leave a macro.
Agree. Fixed with the next patch.
>
> >
> > /* GL_ARB_sync */
> > consts->MaxServerWaitTimeout = 0x1fff7fffffffULL; diff --git
> > a/src/mesa/main/macros.h b/src/mesa/main/macros.h index
> > cd5f2d6..4d245e1 100644
> > --- a/src/mesa/main/macros.h
> > +++ b/src/mesa/main/macros.h
> > @@ -170,27 +170,6 @@ 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) -{
> > - fi_type tmp;
> > - tmp.i = i;
> > - return tmp.f;
> > -}
> > -
> > -static inline GLfloat UINT_AS_FLT(GLuint u) -{
> > - fi_type tmp;
> > - tmp.u = u;
> > - return tmp.f;
> > -}
> > -
> > -static inline unsigned FLT_AS_UINT(float f) -{
> > - fi_type tmp;
> > - tmp.f = f;
> > - return tmp.u;
> > -}
> > -
> > /**
> > * Convert a floating point value to an unsigned fixed point value.
> > *
> > @@ -625,15 +604,11 @@ COPY_CLEAN_4V_TYPE_AS_FLOAT(GLfloat dst[4], int
> > sz, const GLfloat src[4], {
> > switch (type) {
> > case GL_FLOAT:
> > - ASSIGN_4V(dst, 0, 0, 0, 1);
> > + ASSIGN_4V(dst, 0.0f, 0.0f, 0.0f, 1.0f);
> > break;
> > case GL_INT:
> > - ASSIGN_4V(dst, INT_AS_FLT(0), INT_AS_FLT(0),
> > - INT_AS_FLT(0), INT_AS_FLT(1));
> > - 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, 0, 0, 0, 1);
> > break;
>
> I'm having trouble understanding how this is correct. This makes all three
> cases the same. They all assign float values {0, 0, 0, 1} to dst.
> Code later in the function (not shown in the patch) then copies possibly
> integer or unsigned values into some of the components. You then end up with
> a mix of integer values and floating point values.
>
> It seems like this function should take two gl_constant_value as parameters
> instead of GLfloat[4].
Seems here is a similar trouble like before.(
Marek comments on this : Integer one is equal to floating-point zero. Floating-point one is equal to 0x3f800000
I back to macros see last patch sent(v1).)
>
> > 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..a853cb1 100644
> > --- a/src/mesa/vbo/vbo_attrib_tmp.h
> > +++ b/src/mesa/vbo/vbo_attrib_tmp.h
> > @@ -28,6 +28,41 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
> > #include "util/u_format_r11g11b10f.h"
> > #include "main/varray.h"
> >
> > +#include "program/prog_parameter.h"
> > +
> > +
> > +static union gl_constant_value UINT_AS_UNION(GLuint u) {
> > + union gl_constant_value tmp;
> > + tmp.u = u;
> > + return tmp;
> > +}
> > +
> > +static inline union gl_constant_value INT_AS_UNION(GLint i) {
> > + union gl_constant_value tmp;
> > + tmp.i = i;
> > + return tmp;
> > +}
> > +
> > +static inline union gl_constant_value FLOAT_AS_UNION(GLfloat f) {
> > + union gl_constant_value tmp;
> > + tmp.f = f;
> > + return tmp;
> > +}
> > +
> > +/* 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 +76,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
> > +91,8 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
> >
> > /* uint */
> > #define ATTRUI( A, N, X, Y, Z, W) ATTR( A, N, GL_UNSIGNED_INT, \
> > - UINT_AS_FLT(X), UINT_AS_FLT(Y), \
> > - UINT_AS_FLT(Z), UINT_AS_FLT(W) )
> > + X, Y, \
> > + Z, W )
> >
> > #define ATTR2UIV( A, V ) ATTRUI( A, 2, (V)[0], (V)[1], 0, 1 )
> > #define ATTR3UIV( A, V ) ATTRUI( A, 3, (V)[0], (V)[1], (V)[2], 1 )
> > diff --git a/src/mesa/vbo/vbo_exec.h b/src/mesa/vbo/vbo_exec.h index
> > bb265de..7db6b6b 100644
> > --- a/src/mesa/vbo/vbo_exec.h
> > +++ b/src/mesa/vbo/vbo_exec.h
> > @@ -38,6 +38,7 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
> > #include "vbo.h"
> > #include "vbo_attrib.h"
> >
> > +#include "program/prog_parameter.h"
> >
> > /**
> > * Max number of primitives (number of glBegin/End pairs) per VBO.
> > @@ -104,7 +105,7 @@ struct vbo_exec_context
> > GLenum attrtype[VBO_ATTRIB_MAX];
> > GLubyte active_sz[VBO_ATTRIB_MAX];
> >
> > - GLfloat *attrptr[VBO_ATTRIB_MAX];
> > + gl_constant_value *attrptr[VBO_ATTRIB_MAX];
> > struct gl_client_array arrays[VERT_ATTRIB_MAX];
> >
> > /* According to program mode, the values above plus current
> > diff --git a/src/mesa/vbo/vbo_exec_api.c b/src/mesa/vbo/vbo_exec_api.c
> > index 5f8250e..b6c1055 100644
> > --- a/src/mesa/vbo/vbo_exec_api.c
> > +++ b/src/mesa/vbo/vbo_exec_api.c
> > @@ -46,7 +46,6 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
> > #include "vbo_context.h"
> > #include "vbo_noop.h"
> >
> > -
> > #ifdef ERROR
> > #undef ERROR
> > #endif
> > @@ -163,7 +162,7 @@ static void vbo_exec_copy_to_current( struct
> > vbo_exec_context *exec )
> >
> > COPY_CLEAN_4V_TYPE_AS_FLOAT(tmp,
> > exec->vtx.attrsz[i],
> > - exec->vtx.attrptr[i],
> > + (GLfloat*)exec->vtx.attrptr[i],
> > exec->vtx.attrtype[i]);
> >
> > if (exec->vtx.attrtype[i] != vbo->currval[i].Type || @@
> > -216,10 +215,10 @@ vbo_exec_copy_from_current(struct vbo_exec_context *exec)
> > for (i = VBO_ATTRIB_POS + 1; i < VBO_ATTRIB_MAX; i++) {
> > const GLfloat *current = (GLfloat *) vbo->currval[i].Ptr;
> > switch (exec->vtx.attrsz[i]) {
> > - case 4: exec->vtx.attrptr[i][3] = current[3];
> > - case 3: exec->vtx.attrptr[i][2] = current[2];
> > - case 2: exec->vtx.attrptr[i][1] = current[1];
> > - case 1: exec->vtx.attrptr[i][0] = current[0];
> > + case 4: exec->vtx.attrptr[i][3].f = current[3];
> > + case 3: exec->vtx.attrptr[i][2].f = current[2];
> > + case 2: exec->vtx.attrptr[i][1].f = current[1];
> > + case 1: exec->vtx.attrptr[i][0].f = current[0];
> > break;
> > }
> > }
> > @@ -291,7 +290,7 @@ vbo_exec_wrap_upgrade_vertex(struct
> > vbo_exec_context *exec,
> >
> > for (i = 0 ; i < VBO_ATTRIB_MAX ; i++) {
> > if (exec->vtx.attrsz[i]) {
> > - exec->vtx.attrptr[i] = tmp;
> > + exec->vtx.attrptr[i] = (gl_constant_value*) tmp;
> > tmp += exec->vtx.attrsz[i];
> > }
> > else
> > @@ -305,8 +304,8 @@ vbo_exec_wrap_upgrade_vertex(struct vbo_exec_context
> *exec,
> > }
> > else {
> > /* Just have to append the new attribute at the end */
> > - exec->vtx.attrptr[attr] = exec->vtx.vertex +
> > - exec->vtx.vertex_size - newSize;
> > + exec->vtx.attrptr[attr] = (gl_constant_value*)(exec->vtx.vertex +
> > + exec->vtx.vertex_size - newSize);
> > }
> >
> > /* Replay stored vertices to translate them @@ -327,7 +326,7 @@
> > vbo_exec_wrap_upgrade_vertex(struct vbo_exec_context *exec,
> >
> > if (sz) {
> > GLint old_offset = old_attrptr[j] - exec->vtx.vertex;
> > - GLint new_offset = exec->vtx.attrptr[j] - exec->vtx.vertex;
> > + GLint new_offset = (GLfloat*)exec->vtx.attrptr[j] -
> > +exec->vtx.vertex;
> >
> > if (j == attr) {
> > if (oldSize) {
> > @@ -383,7 +382,7 @@ vbo_exec_fixup_vertex(struct gl_context *ctx, GLuint
> attr, GLuint newSize)
> > * zeros. Don't need to flush or wrap.
> > */
> > for (i = newSize; i <= exec->vtx.attrsz[attr]; i++)
> > - exec->vtx.attrptr[attr][i-1] = id[i-1];
> > + exec->vtx.attrptr[attr][i-1].f = id[i-1];
> > }
> >
> > exec->vtx.active_sz[attr] = newSize; @@ -401,7 +400,7 @@
> > vbo_exec_fixup_vertex(struct gl_context *ctx, GLuint attr, GLuint newSize)
> > * This macro is used to implement all the glVertex, glColor, glTexCoord,
> > * glVertexAttrib, etc functions.
> > */
> > -#define ATTR( A, N, T, V0, V1, V2, V3 ) \
> > +#define ATTR_UNION( A, N, T, V0, V1, V2, V3 )
> \
> > do { \
> > struct vbo_exec_context *exec = &vbo_context(ctx)->exec; \
> > \
> > @@ -412,7 +411,7 @@ do {
> \
> > vbo_exec_fixup_vertex(ctx, A, N); \
> > \
> > { \
> > - GLfloat *dest = exec->vtx.attrptr[A]; \
> > + gl_constant_value *dest = exec->vtx.attrptr[A];
> \
> > if (N>0) dest[0] = V0; \
> > if (N>1) dest[1] = V1; \
> > if (N>2) dest[2] = V2; \
> > diff --git a/src/mesa/vbo/vbo_exec_eval.c
> > b/src/mesa/vbo/vbo_exec_eval.c index 82f89b9..670e763 100644
> > --- a/src/mesa/vbo/vbo_exec_eval.c
> > +++ b/src/mesa/vbo/vbo_exec_eval.c
> > @@ -33,6 +33,18 @@
> > #include "vbo_exec.h"
> >
> >
> > +/** Copy \p SZ elements into a 4-element vector */ #define
> > +COPY_UNION_SZ_4V(DST, SZ, SRC) \
> > +do { \
> > + switch (SZ) { \
> > + case 4: (DST)[3].f = (SRC)[3]; \
> > + case 3: (DST)[2].f = (SRC)[2]; \
> > + case 2: (DST)[1].f = (SRC)[1]; \
> > + case 1: (DST)[0].f = (SRC)[0]; \
> > + } \
> > +} while(0)
> > +
> > +
> > static void clear_active_eval1( struct vbo_exec_context *exec, GLuint
> > attr ) {
> > assert(attr < Elements(exec->eval.map1)); @@ -138,9 +150,9 @@ void
> > vbo_exec_do_EvalCoord1f(struct vbo_exec_context *exec, GLfloat u)
> > exec->eval.map1[attr].sz,
> > map->Order);
> >
> > - COPY_SZ_4V( exec->vtx.attrptr[attr],
> > - exec->vtx.attrsz[attr],
> > - data );
> > + COPY_UNION_SZ_4V( exec->vtx.attrptr[attr],
> > + exec->vtx.attrsz[attr],
> > + data );
> > }
> > }
> >
> > @@ -186,7 +198,7 @@ void vbo_exec_do_EvalCoord2f( struct vbo_exec_context
> *exec,
> > exec->eval.map2[attr].sz,
> > map->Uorder, map->Vorder);
> >
> > - COPY_SZ_4V( exec->vtx.attrptr[attr],
> > + COPY_UNION_SZ_4V( exec->vtx.attrptr[attr],
> > exec->vtx.attrsz[attr],
> > data );
> > }
> > @@ -225,7 +237,7 @@ void vbo_exec_do_EvalCoord2f( struct vbo_exec_context
> *exec,
> > NORMALIZE_3FV(normal);
> > normal[3] = 1.0;
> >
> > - COPY_SZ_4V( exec->vtx.attrptr[VBO_ATTRIB_NORMAL],
> > + COPY_UNION_SZ_4V( exec->vtx.attrptr[VBO_ATTRIB_NORMAL],
> > exec->vtx.attrsz[VBO_ATTRIB_NORMAL],
> > normal );
> >
> > diff --git a/src/mesa/vbo/vbo_save_api.c b/src/mesa/vbo/vbo_save_api.c
> > index 5055c22..2a43f15 100644
> > --- a/src/mesa/vbo/vbo_save_api.c
> > +++ b/src/mesa/vbo/vbo_save_api.c
> > @@ -769,7 +769,7 @@ _save_reset_vertex(struct gl_context *ctx)
> > * 3f version won't otherwise set color[3] to 1.0 -- this is the job
> > * of the chooser function when switching between Color4f and Color3f.
> > */
> > -#define ATTR(A, N, T, V0, V1, V2, V3) \
> > +#define ATTR_UNION(A, N, T, V0, V1, V2, V3) \
> > do { \
> > struct vbo_save_context *save = &vbo_context(ctx)->save; \
> > \
> > @@ -778,10 +778,10 @@ do {
> \
> > \
> > { \
> > GLfloat *dest = save->attrptr[A]; \
> > - if (N>0) dest[0] = V0; \
> > - if (N>1) dest[1] = V1; \
> > - if (N>2) dest[2] = V2; \
> > - if (N>3) dest[3] = V3; \
> > + if (N>0) dest[0] = V0.f; \
> > + if (N>1) dest[1] = V1.f; \
> > + if (N>2) dest[2] = V2.f; \
> > + if (N>3) dest[3] = V3.f; \
> > save->attrtype[A] = T; \
> > } \
> > \
> >
More information about the mesa-dev
mailing list