Am 16.01.2018 um 16:49 schrieb Brian Paul:
> On 01/15/2018 07:18 PM, srol...@vmware.com wrote:
>> From: Roland Scheidegger <srol...@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 <bri...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
