[Mesa-dev] [PATCH 3/8] nir: add a helper for getting the bitmask for a variable's location

[Kenneth Graunke]

On Sunday, September 24, 2017 4:56:00 PM PDT Timothy Arceri wrote:
> On 23/09/17 04:33, Kenneth Graunke wrote:
[snip]
> > This is duplicating the code in nir_gather_info.c pretty hard, but it
> > doesn't handle patch variables, or scalar arrays with var->data.compact
> > set (i.e. gl_ClipDistance as float[8] packed as if it were vec4[2]).
> 
> We don't try to remove builtins so var->data.compact shouldn't be a 
> problem right?

Yes, it's not needed for your current use of this function.

However, you're introducing a function that looks generally useful
but gets the wrong answer for a lot of corner cases.  That's kind of
setting a trap for future developers to write bugs, which although
sometimes unavoidable, is still a nice thing to try and avoid...

We literally had this exact function at one point and ended up having
to delete it because it didn't work for a bunch of things people were
trying to use it for.  I've fixed more bugs than I care to count here.

> > Would it make sense to wrap or refactor set_io_mask()?
> 
> Maybe, I'm not sure. There is a bunch of stuff in nir_gather_info.c we 
> don't really need just yet. For example we currently make no attempt to 
> remove components from partially used arrays.
> 
> I'd really like to land this first, which with your suggestion above is 
> really a very small amount of code. From here I'm working on a NIR 
> vector splitting pass and small update to this series to handle removal 
> per component rather than per location. Initial results (take with a 
> gain of salt as I still have a handful of piglit issues) are looking 
> very promising.
> 
> total instructions in shared programs: 13190730 -> 13110599 (-0.61%)
> instructions in affected programs: 2110903 -> 2030772 (-3.80%)
> helped: 14043
> HURT: 486
> 
> total cycles in shared programs: 541148990 -> 540561942 (-0.11%)
> cycles in affected programs: 290344296 -> 289757248 (-0.20%)
> helped: 23418
> HURT: 11623
> 
> Once I write the NIR packing pass this should get even better.
> 
> I'd really like it if we could just land this patch as is then come back 
> and improve things once a more full linking implementation is in place 
> rather than trying to get it perfect in one step. Does that seem reasonable?

Fine.  The nir_gather_info pass does have a whole bunch of partial
mucking about, which is unnecessary (and even harmful) for what you're
doing.  I'm still unhappy with the duplication, but the gather code is
kind of a mess, and you've convinced me that it might be a pain to
clean it up.

So, with the nir_is_per_vertex_io change,
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170925/c352ee05/attachment.sig>