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 <kenn...@whitecape.org>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
