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

[Brian Paul]

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.

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.


>
>      /* 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>