> -----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 = 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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > - Neil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
