----- Original Message ----- > On 01/28/2013 03:45 PM, srol...@vmware.com wrote: > > From: Roland Scheidegger<srol...@vmware.com> > > > > The struct padding got broken by > > c789b981b244333cfc903bcd1e2fefc010500013. > > This caused serious performance regression because part of the key > > was > > unitialized and hence the shader always recompiled (at least on > > release > > builds...). > > While here also fix key size calculation when the number of > > samplers > > and the number of sampler views are different. > > --- > > src/gallium/auxiliary/draw/draw_llvm.c | 3 ++- > > src/gallium/auxiliary/draw/draw_llvm.h | 3 ++- > > src/gallium/auxiliary/draw/draw_vs_llvm.c | 5 +++-- > > 3 files changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/src/gallium/auxiliary/draw/draw_llvm.c > > b/src/gallium/auxiliary/draw/draw_llvm.c > > index afb10a6..dc83f80 100644 > > --- a/src/gallium/auxiliary/draw/draw_llvm.c > > +++ b/src/gallium/auxiliary/draw/draw_llvm.c > > @@ -1378,7 +1378,8 @@ draw_llvm_make_variant_key(struct draw_llvm > > *llvm, char *store) > > key->clip_halfz = > > !llvm->draw->rasterizer->gl_rasterization_rules; > > key->need_edgeflags = (llvm->draw->vs.edgeflag_output ? TRUE : > > FALSE); > > key->ucp_enable = llvm->draw->rasterizer->clip_plane_enable; > > - key->pad = 0; > > + key->pad1 = 0; > > + key->pad2 = 0; > > > > /* All variants of this shader will have the same value for > > * nr_samplers. Not yet trying to compact away holes in the > > diff --git a/src/gallium/auxiliary/draw/draw_llvm.h > > b/src/gallium/auxiliary/draw/draw_llvm.h > > index a664857..8e5bd3c 100644 > > --- a/src/gallium/auxiliary/draw/draw_llvm.h > > +++ b/src/gallium/auxiliary/draw/draw_llvm.h > > @@ -206,8 +206,9 @@ struct draw_llvm_variant_key > > unsigned clip_halfz:1; > > unsigned bypass_viewport:1; > > unsigned need_edgeflags:1; > > + unsigned pad1:1; > > unsigned ucp_enable:PIPE_MAX_CLIP_PLANES; > > - unsigned pad:33-PIPE_MAX_CLIP_PLANES; > > + unsigned pad2:32-PIPE_MAX_CLIP_PLANES; > > There should probably be a prominent comment on this struct about the > importance of zeroing-out any unused bits, adding padding fields, > etc. > to help prevent this in the future.
Yes, I agree. > If we wanted to be really safe, during init we could create a dummy > key, set it to all ones, then set all of the fields to zero, then > assert that no bits in the key are still set. Unfortunately that doesn't catch all the cases: during structure assignment (via the = operator) the compiler is fre to clobber (or copy) any unpadded bits. The actual behavior depends on compiler, flags, code, etc. Another alternative is to never use the = operator, and instead use memcpy operator, but that loses type checking, so it has its own share of problems. I think that add padding and add big warnings as you suggest is the best way to go. > > /* Variable number of vertex elements: > > */ > > diff --git a/src/gallium/auxiliary/draw/draw_vs_llvm.c > > b/src/gallium/auxiliary/draw/draw_vs_llvm.c > > index 3e46f8c..ac3999e 100644 > > --- a/src/gallium/auxiliary/draw/draw_vs_llvm.c > > +++ b/src/gallium/auxiliary/draw/draw_vs_llvm.c > > @@ -100,8 +100,9 @@ draw_create_vs_llvm(struct draw_context *draw, > > > > vs->variant_key_size = > > draw_llvm_variant_key_size( > > - vs->base.info.file_max[TGSI_FILE_INPUT]+1, > > - vs->base.info.file_max[TGSI_FILE_SAMPLER]+1); > > + vs->base.info.file_max[TGSI_FILE_INPUT]+1, > > + MAX2(vs->base.info.file_max[TGSI_FILE_SAMPLER]+1, > > + vs->base.info.file_max[TGSI_FILE_SAMPLER_VIEW]+1)); > > > > vs->base.state.stream_output = state->stream_output; > > vs->base.draw = draw; > > Otherwise, > > Reviewed-by: Brian Paul <bri...@vmware.com> Reviewed-by: Jose Fonseca <jfons...@vmware.com> Jose _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
