[Mesa-dev] [PATCH] draw: fix draw_llvm_variant_key struct padding to avoid recompiles

[Jose Fonseca]

----- Original Message -----
> On 01/28/2013 03:45 PM, sroland at vmware.com wrote:
> > From: Roland Scheidegger<sroland at 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 <brianp at vmware.com>

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>

Jose