[Mesa-dev] [PATCH 090/133] nir: Add a copy splitting pass

[Connor Abbott]

On Wed, Dec 17, 2014 at 9:38 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>
> On Wed, Dec 17, 2014 at 6:01 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>
>> > +
>> > +static nir_deref *
>> > +get_deref_tail(nir_deref *deref)
>> > +{
>> > +   while (deref->child != NULL)
>> > +      deref = deref->child;
>> > +   return deref;
>> > +}
>>
>> I think long-term, we should probably make dereferences exec lists so
>> we don't have to do things like this. In the meantime, though, I
>> could've sworn I had to write this exact same function somewhere else
>> (glsl to nir?)... might be a good idea to move this to nir.c and reuse
>> it, unless nobody else needs this.
>
>
> I also thought about making them a single block of data and dropping the
> linked list thing entirely.  It always has to start with a variable and the
> only difference between an array deref and a structure deref is that an
> array deref can have an indirect/wildcard.  This would make it a little more
> awkward to build the deref chains, but they would take less memory and
> copying/iterating would be easier.  That said, I haven't written the patch
> yet, so I don't know how hard it would be.

Yeah, a lot of things would be better than what we have now. I'm still
leaning a bit towards linked lists rather than arrays, but if you can
come up with something that makes functions like
nir_split_var_copy_instr() easier to write I'd be happy too.

>
> And yes, I copied that function from lower_variables_scalar.
>
>>
>>
>> > +
>> > +static void
>> > +nir_split_var_copy_instr(nir_intrinsic_instr *old_copy,
>> > +                         nir_deref *dest_head, nir_deref *src_head,
>> > +                         nir_deref *dest_tail, nir_deref *src_tail,
>> > +                         struct split_var_copies_state *state)
>>
>> It took me a while to see exactly what this is doing, why we need
>> separate dest_tail and src_tail variables even though they're almost
>> always the same, and how it creates this temporary deref chain that's
>> shared between source and destination (a no-no in NIR) but that's ok
>> because they both get copied at the very end of the recursion when we
>> actually emit the instructions. I'd like to see a comment explaining
>> how this function works that explains those things.
>
>
> Sure.  A comment would be a good plan.  Also, I do modify deref chains in
> several of my patches.  Sometimes I even set them back.  Yes, this is
> probably bad form.  However, the kind of iteration we're doing almost
> requires it.  I'd like to come up with a better plan for that at some point,
> but I haven't found one yet.
> --Jason

Yeah, although at least to me, doing "modify the variable; recurse;
un-modify it" is a lot more obvious than what's going on here. I know
dealing with this stuff is painful... it's sort of a relic of GLSL IR,
but I haven't thought of a better way to do it either. At least it's
fairly contained, in that only a limited number of optimization and
lowering passes will have to deal with it.