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

[Brian Paul]

On 01/16/2018 09:46 AM, Roland Scheidegger wrote:
> 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.

I didn't actually notice that difference but it looks easy enough to 
work around.


> But I quite dislike such macros in any case. Getting rid of the macro
> entirely sounds good to me though.

OK.

-Brian