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

[Jason Ekstrand]

On Tue, Aug 21, 2018 at 8:09 PM Caio Marcelo de Oliveira Filho <
caio.oliveira at intel.com> wrote:

> 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.
>

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.


> > 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?
>

Yes, all variables will likely disappear.


> > > > +      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)))
> {
>

Sure, I can do that.  I actually moved it into a helper because those lines
were getting long.

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180822/a9f89d11/attachment-0001.html>