[Mesa-dev] [PATCH v2] Fixing an x86 FPU bug.
Predut, Marius
marius.predut at intel.com
Tue Feb 10 12:20:54 PST 2015
> -----Original Message-----
> From: Neil Roberts [mailto:neil at linux.intel.com]
> Sent: Wednesday, February 04, 2015 4:02 PM
> To: Predut, Marius; mesa-dev at lists.freedesktop.org
> Cc: mesa-stable at 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.predut at intel.com writes:
>
> > 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).
> > The defect was checked with and without -O3 compiler flag.
> >
> > CC: <mesa-stable at lists.freedesktop.org>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668
> > Signed-off-by: Marius Predut <marius.predut at 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 = 0x1fff7fffffffULL; 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 it's only
> used for fixed constant values but it seems like a bad idea to introduce a
> problematic macro in case someone uses it for other purposes later.
Agree. No much to change here.
>
> > /*@}*/
> >
> >
> > @@ -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)
>
> It might make sense to change the name of this macro now that it doesn't
> actually copy as float anymore.
Ok
>
> > {
> > switch (type) {
> > case GL_FLOAT:
> > - ASSIGN_4V(dst, 0, 0, 0, 1);
> > + ASSIGN_4V_UNION(dst, 0, 0, 0, 1);
>
> This could use the ASSIGN_4V macro instead if all of the arguments were
> wrapped in FLOAT_AS_UNION.
>
> > 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);
>
> If the ASSIGN_4V macro was used instead then this could be the same but
> without the .f members on the arguments.
>
> > 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, 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, 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..a61efde 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
> > @@ -159,7 +158,7 @@ static void vbo_exec_copy_to_current( struct
> vbo_exec_context *exec )
> > * ctx->Current.Attrib and ctx->Light.Material.Attrib arrays.
> > */
> > GLfloat *current = (GLfloat *)vbo->currval[i].Ptr;
> > - GLfloat tmp[4];
> > + gl_constant_value tmp[4];
> >
> > COPY_CLEAN_4V_TYPE_AS_FLOAT(tmp,
> > exec->vtx.attrsz[i], @@ -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];
>
> This looks problematic because it is copying via a float. I think it would be
> better to make current be a pointer to a gl_constant_value and then cast vbo-
> >currval[i].Ptr to that instead. That way you wouldn't need to change the case
> statements and it wouldn't copy via a float.
>
Will do
> > 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;
>
> I think a more complete patch would be to also change
> vbo_exec_context::vtx.vertex to be an array of gl_constant_value instead of
> floats as that would make it more clear that integers may be stored there.
> That way this tmp variable could also be a gl_constant_value and we wouldn't
> need this cast.
>
> > 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);
>
> Same goes here, if exec->vtx.vertex were changed to be a gl_constant_value
> then this cast wouldn't be necessary.
>
> > }
> >
> > /* Replay stored vertices to translate them @@ -327,15 +326,15 @@
> > 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;
>
> Same here.
>
> >
> > if (j == attr) {
> > if (oldSize) {
> > - GLfloat tmp[4];
> > + gl_constant_value tmp[4];
> > COPY_CLEAN_4V_TYPE_AS_FLOAT(tmp, oldSize,
> > - data + old_offset,
> > +
> > + (gl_constant_value*)(data + old_offset),
>
> I think it would be good to change exec->vtx.copied.buffer and
> exec->btx.buffer_ptr to be a gl_constant_value as well so that we could
> avoid this cast.
Those changes involve others more changes(vbo_save_map_vertex_store, _save_copy_vertices, struct vbo_save_copied_vtx, etc)_
>
> > exec->vtx.attrtype[j]);
> > - COPY_SZ_4V(dest + new_offset, newSize, tmp);
> > + COPY_SZ_4V(dest + new_offset, newSize, (GLfloat*)tmp);
> > } else {
> > GLfloat *current = (GLfloat *)vbo->currval[j].Ptr;
> > COPY_SZ_4V(dest + new_offset, sz, current); @@ -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];
>
> This is copying as a float so it looks like it will have the same bug.
> We should probably change vbo_get_default_vals_as_float to be
> vbo_get_default_vals_as_union or something and make it return a pointer to
> const gl_constant_value *
>
Ok
> > }
> >
> > 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)
> > +
> > +
>
> I think this macro could do with a better name because as it stands it looks
> like it is just bringing back the problematic macro to copy via floats.
> However I think a better idea would be to avoid the macro altogether and do as
> described below.
>
I will try to remove it.
> > 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 );
>
> If we make data be an array of gl_constant_values then we can pass &data[0].f
> to _math_horner_bezier_curve and then continue to use COPY_SZ_4V.
>
Nice solution!
> > }
> > }
> >
> > @@ -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 );
>
> Simiarly here _math_horner_bezier_surf could be passed &data[0].f.
>
> > }
> > @@ -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 );
>
> And here normal could be gl_constant_value and you could pass &normal[0].f to
> CROSS3 and NORMALIZE_3FV.
>
> >
> > diff --git a/src/mesa/vbo/vbo_save_api.c b/src/mesa/vbo/vbo_save_api.c
> > index 5055c22..c24adde 100644
> > --- a/src/mesa/vbo/vbo_save_api.c
> > +++ b/src/mesa/vbo/vbo_save_api.c
> > @@ -575,8 +575,8 @@ _save_copy_to_current(struct gl_context *ctx)
> > for (i = VBO_ATTRIB_POS + 1; i < VBO_ATTRIB_MAX; i++) {
> > if (save->attrsz[i]) {
> > save->currentsz[i][0] = save->attrsz[i];
> > - COPY_CLEAN_4V_TYPE_AS_FLOAT(save->current[i], save->attrsz[i],
> > - save->attrptr[i], save->attrtype[i]);
> > + COPY_CLEAN_4V_TYPE_AS_FLOAT((gl_constant_value*)save->current[i],
> save->attrsz[i],
> > +
> > + (gl_constant_value*)save->attrptr[i], save->attrtype[i]);
>
> Again it would be more complete to change vbo_save_context::current and
> attrptr to be pointers to gl_constant_value rather than casting it everywhere.
>
Will do. But more changes need
> > }
> > }
> > }
> > @@ -682,7 +682,7 @@ _save_upgrade_vertex(struct gl_context *ctx, GLuint
> attr, GLuint newsz)
> > if (save->attrsz[j]) {
> > if (j == attr) {
> > if (oldsz) {
> > - COPY_CLEAN_4V_TYPE_AS_FLOAT(dest, oldsz, data,
> > +
> > + COPY_CLEAN_4V_TYPE_AS_FLOAT((gl_constant_value*)dest, oldsz,
> > + (gl_constant_value*)data,
>
> save->copied.buffer and save->buffer could be changed to a
> gl_constant_value as well.
Ok.
>
> > save->attrtype[j]);
> > data += oldsz;
> > dest += newsz;
> > @@ -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; \
>
> This is copying via a float again. save->attrptr should be changed to a
> gl_constant_value like you have already done for exec->attrptr.
Agree
>
> > save->attrtype[A] = T; \
> > } \
> > \
> > diff --git a/src/mesa/vbo/vbo_save_draw.c
> > b/src/mesa/vbo/vbo_save_draw.c index d0521d7..d923403 100644
> > --- a/src/mesa/vbo/vbo_save_draw.c
> > +++ b/src/mesa/vbo/vbo_save_draw.c
> > @@ -76,11 +76,11 @@ _playback_copy_to_current(struct gl_context *ctx,
> > for (i = VBO_ATTRIB_POS+1 ; i < VBO_ATTRIB_MAX ; i++) {
> > if (node->attrsz[i]) {
> > GLfloat *current = (GLfloat *)vbo->currval[i].Ptr;
> > - GLfloat tmp[4];
> > + gl_constant_value tmp[4];
> >
> > COPY_CLEAN_4V_TYPE_AS_FLOAT(tmp,
> > node->attrsz[i],
> > - data,
> > + (gl_constant_value*)data,
> > node->attrtype[i]);
> >
>
> I guess it would also make sense to change vbo_save_vertex_list->current_data
> to be a gl_constant_value so we can avoid this cast too.
I will avoid it by using 2 versions for COPY_CLEAN_4V_TYPE_AS_FLOAT
One with gl_constat_value and one with GLfloat
>
> > if (node->attrtype[i] != vbo->currval[i].Type ||
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> - Neil
More information about the mesa-dev
mailing list