On Tue, Aug 21, 2018 at 8:09 PM Caio Marcelo de Oliveira Filho <
caio.olive...@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.olive...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
