[Mesa-dev] [PATCH v2 08/11] nir: Add a array-of-vector variable shrinking pass

[Caio Marcelo de Oliveira Filho]

On Tue, Aug 21, 2018 at 06:15:20PM -0500, Jason Ekstrand wrote:
> On Tue, Aug 21, 2018 at 5:55 PM Caio Marcelo de Oliveira Filho <
> caio.oliveira at intel.com> wrote:
> 
> > Hi,
> >
> > On Sat, Jul 28, 2018 at 10:44:39PM -0700, Jason Ekstrand wrote:
> > > This pass looks for variables with vector or array-of-vector types and
> > > narrows the type to only the components used.
> > > ---
> > >  src/compiler/nir/nir.h            |   1 +
> > >  src/compiler/nir/nir_split_vars.c | 694 ++++++++++++++++++++++++++++++
> > >  2 files changed, 695 insertions(+)
> >
> > My general impression was: could we do it as two separate simpler
> > passes, one for array lengths and other for vector types?  I felt the
> > intermix of book-keeping for both cases made things harder than usual
> > to read, but maybe is only because I'm not familiar enough.
> >
> 
> We probably could.  However, the logic used in the two types of shrinking
> is basically exactly the same so it made sense to put them together and not
> repeat everything.  At one point, I had array shrinking mixed in with array
> splitting and that was a terrible idea as they're very different.  These
> two, however, are basically the same only with a max length vs. a component
> mask.

I'd guess we could find better opportunities to "shrink" the structure
of the code with each of the cases separate, but I see your point.


> > > +         case nir_intrinsic_copy_deref: {
> > > +            /* Just mark everything used for copies. */
> > > +            nir_deref_instr *dst = nir_src_as_deref(intrin->src[0]);
> > > +            nir_deref_instr *src = nir_src_as_deref(intrin->src[1]);
> >
> > Should self-copies be ignored?
> >
> 
> Maybe?  If it's really a self-copy where src == dst, that would be dumb and
> we should delete it.  I'm not sure if any optimization pass already does
> this or not.  If it's a copy from one array element to a different one, we

We do eliminate self copies.  Need to check if it happens before or
after the splits.


> could probably do something better here at least for vector components.
> I'd have to think about it a bit more to figure out what the optimal thing
> there would be.  In any case, doing the conservative thing is still correct.

OK.


> > > +      for (unsigned i = 0; i < usage->num_levels; i++) {
> > > +         struct array_level_usage *level = &usage->levels[i];
> > > +         assert(level->array_len > 0);
> > > +
> > > +         if (level->max_written == UINT_MAX || level->has_external_copy)
> > > +            continue; /* Can't shrink */
> > > +
> > > +         unsigned max_used = MIN2(level->max_read, level->max_written);
> >
> > Should this be MAX2?  What I'm thinking is if we read 3 and write 5,
> > we used up to 5, and the length should be at least 6.
> >
> 
> No, it shouldn't. :-)  If we only read up to 3 but we write up to 5 then
> those last two writes are never read so they can be discarded.  Similarly,
> if we read up to 5 but only write up to 3 then those top two reads read
> garbage and can be replaced with undefs.  Similarly, for components, we AND
> instead of OR for the same reason.  See also the comment above this loop.

You are correct.  Thanks for the clarification. :-)

Optional: add a oneliner comment here highlighting this.


> > > +   bool has_vars_to_shrink = false;
> > > +   nir_foreach_function(function, shader) {
> > > +      if (!function->impl)
> > > +         continue;
> > > +
> > > +      /* Don't even bother crawling the IR if we don't have any
> > variables.
> > > +       * Given that this pass deletes any unused variables, it's likely
> > that
> > > +       * we will be in this scenario eventually.
> > > +       */
> >
> > The fact that we don't have unused variables doesn't mean we won't
> > have used variables.  Maybe I'm missing some context to fully
> > understand this comment block.
> >
> 
> Hunh?  This is just an early exit for the eventual case where we've managed
> to delete all the variables.  We know if there are no variables that the
> pass will do nothing so we might as well not bother walking the IR.  It's a
> silly optimization but should be very safe.

I didn't do a good job explaining myself :-/.  The code makes total
sense to me.  It's the second sentence in the comment that threw me
off, do we expect all the variables to disappear as result of this and
other passes?


> > > +      if (!exec_list_is_empty(&shader->globals) ||
> > > +          !exec_list_is_empty(&function->impl->locals)) {
> >
> > Condition each of those with the modes being enabled? I.e. if we do
> > have globals, but we are only working with locals in the pass, we
> > should skip it.
> >
> 
> We could but we eventually lower globals to locals anyway.

Well, no reason to walk the IR until they become locals in that
case.  To be clear I'm suggesting something like

    if (((modes & nir_var_global) && !exec_list_is_empty(&shader->globals)) ||
        ((modes & nir_var_local) && !exec_list_is_empty(&shader->locals))) {


Thanks,
Caio