[Mesa-dev] [PATCH 5/7] nir: support to clone shaders (v2)

[Rob Clark]

On Mon, Oct 26, 2015 at 6:56 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Mon, Oct 26, 2015 at 12:53 PM, Rob Clark <robdclark at gmail.com> wrote:
>> On Mon, Oct 26, 2015 at 2:44 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>> On Mon, Oct 26, 2015 at 8:27 AM, Rob Clark <robdclark at gmail.com> wrote:
>>>>
>>>> +static void *
>>>> +clone_intrinsic(clone_state *state, const void *ptr)
>>>> +{
>>>> +   const nir_intrinsic_instr *itr = ptr;
>>>> +   unsigned num_srcs = nir_intrinsic_infos[itr->intrinsic].num_srcs;
>>>> +
>>>> +   nir_intrinsic_instr *nitr =
>>>> +      ralloc_size(state->ns,
>>>> +                  sizeof(nir_intrinsic_instr) + num_srcs * sizeof(nir_src));
>>>> +   store_ptr(state, nitr, itr);
>>>> +
>>>> +   __clone_instr(state, &nitr->instr, &itr->instr);
>>>> +
>>>> +   nitr->intrinsic = itr->intrinsic;
>>>> +
>>>> +   if (nir_intrinsic_infos[itr->intrinsic].has_dest)
>>>> +      __clone_dst(state, &nitr->dest, &itr->dest);
>>>> +
>>>> +   nitr->num_components = itr->num_components;
>>>> +   memcpy(nitr->const_index, itr->const_index, sizeof(nitr->const_index));
>>>> +
>>>> +   for (unsigned i = 0; i < ARRAY_SIZE(nitr->variables); i++) {
>>>> +      nitr->variables[i] = clone_ptr(state, itr->variables[i], clone_deref_var);
>>>
>>> Derefs don't need to go in the remap table.  They need to use the
>>> variable from it and need to remap any indirect sources, but the deref
>>> doesn't need to go in the table.
>>
>> that goes for all the various places deref's can appear?  (Ie. call,
>> tex instr, etc)?  They are all private to the instruction?  I got a
>> bit lost/impatient in following glsl_to_nir around so I went for the
>> safe thing..
>
> Yes, that's correct.  A deref is always only local to the instruction.
> This is why I suggested passing the instruction into clone_deref
> above.  If you don't need to use clone_ptr on it, then it doesn't
> require passing extra stuff through.

oh, and I see, and nir_validate actually checks some of this stuff..

(possibly wouldn't hurt for it to validate parent memctx a bit more
thorough.. it took a bit of digging to figure out what should be
parent memctx of what and whether I could completely rely on that..
but other topic..)

I guess things that nir_validate asserts, I'd be more comfortable
relying on, vs letting things get broken until someone bothers to run
a debug build w/ NIR_TEST_CLONE=1 ;-)

>> (That said, in the grand scheme of things I don't think there are so
>> many deref objects, so probably doesn't make much difference)
>
> Sure, but it simplifies things if you just create it on the spot and
> pass the instruction through.

maybe.. otoh less things that are handled differently and require
thought to understand why they are handled in a particular way in
nir_clone, vs other fields/ptrs/etc, makes the code easier to
understand and keep up to date.. or at least that is part of my
reasoning about some things..

>>>> +   }
>>>> +
>>>> +   for (unsigned i = 0; i < num_srcs; i++) {
>>>> +      __clone_src(state, &nitr->src[i], &itr->src[i]);
>>>> +   }
>>>> +
>>>> +   return nitr;
>>>> +}
>>>> +

[snip]

>>>> +
>>>> +static void
>>>> +__clone_cf_node_p2(clone_state *state, nir_cf_node *ncf, const nir_cf_node *cf)
>>>> +{
>>>> +   switch (cf->type) {
>>>> +   case nir_cf_node_block:
>>>> +      __clone_block_p2(state, nir_cf_node_as_block(ncf), nir_cf_node_as_block(cf));
>>>> +      break;
>>>> +   case nir_cf_node_if:
>>>> +      __clone_if_p2(state, nir_cf_node_as_if(ncf), nir_cf_node_as_if(cf));
>>>> +      break;
>>>> +   case nir_cf_node_loop:
>>>> +      __clone_loop_p2(state, nir_cf_node_as_loop(ncf), nir_cf_node_as_loop(cf));
>>>> +      break;
>>>> +   case nir_cf_node_function:
>>>> +      __clone_function_impl_p2(state, nir_cf_node_as_function(ncf),
>>>> +                               nir_cf_node_as_function(cf));
>>>
>>> This whole song-and-dance isn't needed.  We already have
>>> nir_foreach_block which will do all of this for you.  To handle the
>>> ifs, we have nir_block_get_following_if().  See also
>>> nir_live_variables for details.
>>
>> but it won't co-iterate two lists at the same time for me ;-)
>
> Sure, but I'm pretty sure you never use it given my comment on
> clone_block_p2 :-)

well, partially.. I still need some fixup in nir_if too, but given
that I simplified things to just keep a list of nir_src's to fixup, I
also don't need the origin nir_if there currently.  (The earlier
version of the patch had more complete ir __clone_foo_v2()'s to
traverse the entire graph to avoid keeping a list of nir_src's to
fixup.)

At any rate, in either case I need to do my own custom traversal to
get at the nir_if's, regardless of whether I need to co-traverse the
original-foo's.  Leaving the co-traversal in place at least simplifies
things later if any other place needs to do a 2nd-pass ptr lookup, if
the need arises in the future.

Maybe I should just throw in some some assert(nfoo == clone_ptr(foo))
so we have some use for the co-traversal until then ;-)

BR,
-R