Re: [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, 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.

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

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev