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

[Roland Scheidegger]

Am 16.01.2018 um 16:49 schrieb 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.
> 
Note that the other 7 lines aren't quite identical - only the ubyte and
ushort functions are, but the uint has a different if condition.
It would be possible to unify the conditions, but I think the generated
code would be worse (since this is all part of nested macro functions,
elt_bias is a fixed 0 there if the value from draw->info was 0). Albeit
I suppose could add another parameter (is_uint or so) for a
VSPLIT_ADD_CACHE() macro.
But I quite dislike such macros in any case. Getting rid of the macro
entirely sounds good to me though.

Roland