[Mesa-dev] [PATCH] draw: fix vsplit code when the (post-bias) index value is -1

[Brian Paul]

On 01/15/2018 07:18 PM, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
> 
> vsplit_add_cache uses the post-bias index for hashing, but the
> vsplit_add_cache_uint/ushort/ubyte ones used the pre-bias index, therefore
> the code for handling the special case (because -1 matches the initialization
> value of the cache) wasn't actually working.
> Commit 78a997f72841310620d18daa9015633343d04db1 actually simplified the
> cache logic somewhat, but it looks like this particular problem carried over
> (and duplicated to the ushort/ubyte cases, since before only uint needed it).
> This could lead to the vsplit cache doing the wrong thing, in particular
> later fetch_info might indicate there are 0 values to fetch. This only really
> affected edge cases which were bogus to begin with, but it could lead to a
> crash with the jit vertex shader, since it cannot handle this case correctly
> (the count loop is always executed at least once and we would not allocate
> any memory for the shader outputs), so add another assert to catch it there.
> ---
>   src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c | 1 +
>   src/gallium/auxiliary/draw/draw_pt_vsplit.c                    | 6 +++---
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
> index c6492a1..5e0c562 100644
> --- a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
> +++ b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
> @@ -368,6 +368,7 @@ llvm_pipeline_generic(struct draw_pt_middle_end *middle,
>      unsigned start_or_maxelt, vid_base;
>      const unsigned *elts;
>   
> +   assert(fetch_info->count > 0);
>      llvm_vert_info.count = fetch_info->count;
>      llvm_vert_info.vertex_size = fpme->vertex_size;
>      llvm_vert_info.stride = fpme->vertex_size;
> diff --git a/src/gallium/auxiliary/draw/draw_pt_vsplit.c b/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> index a68d5bf..3ff077b 100644
> --- a/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> +++ b/src/gallium/auxiliary/draw/draw_pt_vsplit.c
> @@ -133,7 +133,7 @@ vsplit_add_cache_ubyte(struct vsplit_frontend *vsplit, const ubyte *elts,
>      VSPLIT_CREATE_IDX(elts, start, fetch, elt_bias);
>      /* unlike the uint case this can only happen with elt_bias */
>      if (elt_bias && elt_idx == DRAW_MAX_FETCH_IDX && !vsplit->cache.has_max_fetch) {
> -      unsigned hash = fetch % MAP_SIZE;
> +      unsigned hash = elt_idx % MAP_SIZE;
>         vsplit->cache.fetches[hash] = 0;
>         vsplit->cache.has_max_fetch = TRUE;
>      }
> @@ -148,7 +148,7 @@ vsplit_add_cache_ushort(struct vsplit_frontend *vsplit, const ushort *elts,
>      VSPLIT_CREATE_IDX(elts, start, fetch, elt_bias);
>      /* unlike the uint case this can only happen with elt_bias */
>      if (elt_bias && elt_idx == DRAW_MAX_FETCH_IDX && !vsplit->cache.has_max_fetch) {
> -      unsigned hash = fetch % MAP_SIZE;
> +      unsigned hash = elt_idx % MAP_SIZE;
>         vsplit->cache.fetches[hash] = 0;
>         vsplit->cache.has_max_fetch = TRUE;
>      }
> @@ -168,7 +168,7 @@ vsplit_add_cache_uint(struct vsplit_frontend *vsplit, const uint *elts,
>      VSPLIT_CREATE_IDX(elts, start, fetch, elt_bias);
>      /* Take care for DRAW_MAX_FETCH_IDX (since cache is initialized to -1). */
>      if (elt_idx == DRAW_MAX_FETCH_IDX && !vsplit->cache.has_max_fetch) {
> -      unsigned hash = fetch % MAP_SIZE;
> +      unsigned hash = elt_idx % MAP_SIZE;
>         /* force update - any value will do except DRAW_MAX_FETCH_IDX */
>         vsplit->cache.fetches[hash] = 0;
>         vsplit->cache.has_max_fetch = TRUE;
> 

Looks OK to me.
Reviewed-by: Brian Paul <brianp at vmware.com>

Though, I'd suggest some follow-up clean-up.  The VSPLIT_CREATE_IDX() 
macro seems kind of funny since it just encapsulates two lines of code 
but it's used in three functions which have about 7 other lines of 
identical code.  I'd either get rid of VSPLIT_CREATE_IDX or instead 
create a VSPLIT_ADD_CACHE() macro that can represent the entire body of 
those functions.

-Brian