<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Aug 21, 2018 at 8:09 PM Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com">caio.oliveira@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Aug 21, 2018 at 06:15:20PM -0500, Jason Ekstrand wrote:<br>
> On Tue, Aug 21, 2018 at 5:55 PM Caio Marcelo de Oliveira Filho <<br>
> <a href="mailto:caio.oliveira@intel.com" target="_blank">caio.oliveira@intel.com</a>> wrote:<br>
> <br>
> > Hi,<br>
> ><br>
> > On Sat, Jul 28, 2018 at 10:44:39PM -0700, Jason Ekstrand wrote:<br>
> > > This pass looks for variables with vector or array-of-vector types and<br>
> > > narrows the type to only the components used.<br>
> > > ---<br>
> > > src/compiler/nir/nir.h | 1 +<br>
> > > src/compiler/nir/nir_split_vars.c | 694 ++++++++++++++++++++++++++++++<br>
> > > 2 files changed, 695 insertions(+)<br>
> ><br>
> > My general impression was: could we do it as two separate simpler<br>
> > passes, one for array lengths and other for vector types? I felt the<br>
> > intermix of book-keeping for both cases made things harder than usual<br>
> > to read, but maybe is only because I'm not familiar enough.<br>
> ><br>
> <br>
> We probably could. However, the logic used in the two types of shrinking<br>
> is basically exactly the same so it made sense to put them together and not<br>
> repeat everything. At one point, I had array shrinking mixed in with array<br>
> splitting and that was a terrible idea as they're very different. These<br>
> two, however, are basically the same only with a max length vs. a component<br>
> mask.<br>
<br>
I'd guess we could find better opportunities to "shrink" the structure<br>
of the code with each of the cases separate, but I see your point.<br>
<br>
<br>
> > > + case nir_intrinsic_copy_deref: {<br>
> > > + /* Just mark everything used for copies. */<br>
> > > + nir_deref_instr *dst = nir_src_as_deref(intrin->src[0]);<br>
> > > + nir_deref_instr *src = nir_src_as_deref(intrin->src[1]);<br>
> ><br>
> > Should self-copies be ignored?<br>
> ><br>
> <br>
> Maybe? If it's really a self-copy where src == dst, that would be dumb and<br>
> we should delete it. I'm not sure if any optimization pass already does<br>
> this or not. If it's a copy from one array element to a different one, we<br>
<br>
We do eliminate self copies. Need to check if it happens before or<br>
after the splits.<br></blockquote><div><br></div><div>I just found it in copy_prop_vars so it will happen after this pass but then the next optimization loop will have them deleted and be able to split things if it was self-copies that were the problem. Then again, on the first run of the optimization loop, we may not even have run copy-prop yet so this pass isn't all that likely to do anything anyway.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> could probably do something better here at least for vector components.<br>
> I'd have to think about it a bit more to figure out what the optimal thing<br>
> there would be. In any case, doing the conservative thing is still correct.<br>
<br>
OK.<br>
<br>
<br>
> > > + for (unsigned i = 0; i < usage->num_levels; i++) {<br>
> > > + struct array_level_usage *level = &usage->levels[i];<br>
> > > + assert(level->array_len > 0);<br>
> > > +<br>
> > > + if (level->max_written == UINT_MAX || level->has_external_copy)<br>
> > > + continue; /* Can't shrink */<br>
> > > +<br>
> > > + unsigned max_used = MIN2(level->max_read, level->max_written);<br>
> ><br>
> > Should this be MAX2? What I'm thinking is if we read 3 and write 5,<br>
> > we used up to 5, and the length should be at least 6.<br>
> ><br>
> <br>
> No, it shouldn't. :-) If we only read up to 3 but we write up to 5 then<br>
> those last two writes are never read so they can be discarded. Similarly,<br>
> if we read up to 5 but only write up to 3 then those top two reads read<br>
> garbage and can be replaced with undefs. Similarly, for components, we AND<br>
> instead of OR for the same reason. See also the comment above this loop.<br>
<br>
You are correct. Thanks for the clarification. :-)<br>
<br>
Optional: add a oneliner comment here highlighting this.<br>
<br>
<br>
> > > + bool has_vars_to_shrink = false;<br>
> > > + nir_foreach_function(function, shader) {<br>
> > > + if (!function->impl)<br>
> > > + continue;<br>
> > > +<br>
> > > + /* Don't even bother crawling the IR if we don't have any<br>
> > variables.<br>
> > > + * Given that this pass deletes any unused variables, it's likely<br>
> > that<br>
> > > + * we will be in this scenario eventually.<br>
> > > + */<br>
> ><br>
> > The fact that we don't have unused variables doesn't mean we won't<br>
> > have used variables. Maybe I'm missing some context to fully<br>
> > understand this comment block.<br>
> ><br>
> <br>
> Hunh? This is just an early exit for the eventual case where we've managed<br>
> to delete all the variables. We know if there are no variables that the<br>
> pass will do nothing so we might as well not bother walking the IR. It's a<br>
> silly optimization but should be very safe.<br>
<br>
I didn't do a good job explaining myself :-/. The code makes total<br>
sense to me. It's the second sentence in the comment that threw me<br>
off, do we expect all the variables to disappear as result of this and<br>
other passes?<br></blockquote><div><br></div><div>Yes, all variables will likely disappear.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > > + if (!exec_list_is_empty(&shader->globals) ||<br>
> > > + !exec_list_is_empty(&function->impl->locals)) {<br>
> ><br>
> > Condition each of those with the modes being enabled? I.e. if we do<br>
> > have globals, but we are only working with locals in the pass, we<br>
> > should skip it.<br>
> ><br>
> <br>
> We could but we eventually lower globals to locals anyway.<br>
<br>
Well, no reason to walk the IR until they become locals in that<br>
case. To be clear I'm suggesting something like<br>
<br>
if (((modes & nir_var_global) && !exec_list_is_empty(&shader->globals)) ||<br>
((modes & nir_var_local) && !exec_list_is_empty(&shader->locals))) {<br></blockquote><div><br></div><div>Sure, I can do that. I actually moved it into a helper because those lines were getting long.</div><div><br></div><div>--Jason<br></div></div></div>